Conversation
kozlovic
left a comment
There was a problem hiding this comment.
Too much to review. This is obviously very early and more of a POC, but you should still try to run a little benchmark for the publish to see how it will perform. I know that early in the C client development the publish perf was really bad compared to the Go client because each publish ended-up result in the tcp write (because notifying the flusher would do the flush right away). He we copy/move the date to a chain but how often the chain will be flushed/processed will dictate the publish performance.
|
|
||
| // Initialize the connect-time memory pool. | ||
| s = nats_createPool(&nc->connectPool, &nc->opts->mem, "conn-connect"); | ||
| if (s != NATS_OK) | ||
| return NATS_UPDATE_ERR_STACK(s); |
There was a problem hiding this comment.
Should the state set above remain the same or be updated on failure? Or maybe set the state after this possible men failure.
| s = natsSock_ConnectTcp(&(nc->sockCtx), nc->connectPool, nc->cur->url->host, nc->cur->url->port); | ||
| if (STILL_OK(s)) | ||
| nc->sockCtx.fdActive = true; | ||
| if (STILL_OK(s)) |
There was a problem hiding this comment.
Not needed, falls under the above STILL_OK.
|
|
||
| return NATS_UPDATE_ERR_STACK(s); | ||
| } | ||
| natsConn_retain(nc); |
There was a problem hiding this comment.
Comment says that locking is done inside the function itself, but I don't see it happening (setting "nc->state", etc..).
|
@kozlovic ACK that it is way too early to review, and I am focused on other things for the time being anyway. I marked you to get your "FYI"-level attention, feel free to un-assign if this is too noisy.
the plan was to optimize it to use |
PR for discussion only, not to be merged.
natsSock_ConnectTcpto connectwrites are not optimized at all, yet. Turn onDEV_MODE_CONN_TRACEto see the trace.Example: https://github.com/nats-io/nats.c/blob/mininats/examples/libevent-pub.c
It connects and sends 5 messages to a server. The entire heap usage is logged (there are still leaks, please ignore, cleanup is not fully implemented).
To see more details, turn on more logging here
server log: