Add uv_udp_try_send()

[ibc]

For real-time applications in which UDP is mostly used, having uv_udp_try_send() (equivalent to uv_tcp_try_write()) would be very useful as it may avoid buffer memcpy for each send call.

With uv_udp_try_send() we can first try it and, in case it returns zero (so the socket is not ready for writing right now) we can continue with current uv_udp_send() procedures.

Could you please implement this?

[saghul]

Could you please implement this?

Patches are welcome :-)

[ibc]

Yes, I will try. But for me it is very difficult to get deep into UV internal structures.

[txdv]

Streams have a write queue and a field called write_queue_size which exposes how much left there is in the write queue to write.

This field get's used in uv_tcp_try_write to determine if one can write or not. So the first step is to actually expose the same functionality in udp because udp doesn't have this field, but it has a write queue and you can't just write force push something before the write queue is empty.

[ibc]

Great info. I will start with that :)

BTW: in include/uv-unix.h:

#define UV_STREAM_PRIVATE_FIELDS                                              \
  uv_connect_t *connect_req;                                                  \
  uv_shutdown_t *shutdown_req;                                                \
  uv__io_t io_watcher;                                                        \
  void* write_queue[2];                                                       \
  void* write_completed_queue[2];                                             \
  uv_connection_cb connection_cb;                                             \
  int delayed_error;                                                          \
  int accepted_fd;                                                            \
  void* queued_fds;                                                           \
  UV_STREAM_PRIVATE_PLATFORM_FIELDS                                           \

#define UV_TCP_PRIVATE_FIELDS /* empty */

#define UV_UDP_PRIVATE_FIELDS                                                 \
  uv_alloc_cb alloc_cb;                                                       \
  uv_udp_recv_cb recv_cb;                                                     \
  uv__io_t io_watcher;                                                        \
  void* write_queue[2];                                                       \
  void* write_completed_queue[2];

and in uv.h:

#define UV_STREAM_FIELDS                                                      \
  /* number of bytes queued for writing */                                    \
  size_t write_queue_size;                                                    \
  uv_alloc_cb alloc_cb;                                                       \
  uv_read_cb read_cb;                                                         \
  /* private */                                                               \
  UV_STREAM_PRIVATE_FIELDS

struct uv_stream_s {
  UV_HANDLE_FIELDS
  UV_STREAM_FIELDS
};

May I understand why alloc_cb and read_cb are "public" on uv_stream while they are "private" on uv_udp?

[txdv]

No reason. Inconsistency in libuv. Should they be public or private?

I think they should be public, it lets the user change the callbacks without running tcp_read and udp_recv again, but then the callbacks field in the _read/_recv functions would become obsolete? The callbacks in the function arguments are useful for easier FFI, so they have to stay.

Make the callbacks public, so the cool kids can change them and leave them in the _read/_recv arguments so I don't have to fiddle with the handle offsets all the time.

@saghul Any comments?

[txdv]

Here is a write_queue_size in the udp handle for unix: https://github.com/txdv/libuv/commit/b15d71108b8b715b0d3488d70efab2ebc2539639

uv_count_bufs is duplicated in my code though. I think this function should be made public because it is a useful utility function if people use multiple buffers.

@saghul any comments on uv_count_bufs?

[txdv]

I looked at the windows code https://github.com/txdv/libuv/blob/addr-api/src/win/udp.c#L403-L409

And I see that it tries to write the stuff immediately even when using the normal _send function. Care to elaborate @piscisaureus @bnoordhuis @indutny ?

Why does the try_write for unix when windows seems to have this already build in on all handles that windows supports? Even in the tcp code windows tries to write immediately: https://github.com/joyent/libuv/blob/master/src/win/tcp.c#L833-L839

[ibc]

Why the try_write for unix when windows seems to have this already build in on all handles that windows supports?

Ws strongly need the try_write() API. Otherwise we always need to allocate a specific buffer for the write() call, which is not true when using try_write().

[bnoordhuis]

I don't disagree but I won't be the one writing it. :-)

Some random observations:

1. uv_try_write() returns the number of bytes written. That would be kind of superfluous for uv_udp_try_send() because datagram writes are atomic: everything is written or nothing is but partial writes don't happen. A boolean is probably sufficient but then again, returning the number of bytes written doesn't actually hurt and is "bool-y" too.
2. The current uv__udp_send() implementation is kind of inefficient because it always queues the datagram and polls the socket for write readiness first. More efficient would be to try to write immediately when the write queue is empty and only queue when that fails with EAGAIN.

[ibc]

Why is UDP always the poor brother in any I/O library? :(

[saghul]

Why is UDP always the poor brother in any I/O library? :(

I'm not sure what kind of answer you are expecting, so I'm going to go with "patches are welcome".

[ibc]

It was rather a rhetorical question :)

And yes, I will work on this "feature" (more than a patch I think).

[txdv]

1. Should the read_cb and and alloc_cb be public or private in stream and udp? Currently in stream they are public while in udp they are private. IMO they should be public. Reason is above.
2. Should we make uv_count_bufs public? It is useful code and I need it for the udp write_queue_size implementation anyway. Currently it is implemented two times in seperated places win#1 unix#2.

[ibc]

Yes to 1 and 2 :) And it seems that write_queue_size is a MUST in order to add uv_udp_try_send().

[saghul]

Should the read_cb and and alloc_cb be public or private in stream and udp? Currently in stream they are public while in udp they are private. IMO they should be public. Reason is above.

I don't we need to handle that for this PR. Personally I consider them private.

Should we make uv_count_bufs public? It is useful code and I need it for the udp write_queue_size implementation anyway. Currently it is implemented two times in seperated places win#1 unix#2.

We can probably move it to uv-common and rename it to uv__count_bufs, as it's an internal function.

[txdv]

Should uv__count_bufs be inline like the windows code suggests?

[saghul]

We only use it in a couple of places, so I guess it could be inline, yes.

[ibc]

@txdv what is your roadmap? I assume:

1. Add a common uv_count_bufs and include it into UDP.
2. Improve bullet 2 commented by @bnoordhuis (so unix uv_udp_send() becomes more efficient).

Am I right?

[txdv]

1. I will put it into uv_common as a common internal lib function, we can make it public afterwards if it is needed.
2. This change depends on write_queue_size or a write_queue_count.

Now that bnoordhuis mentioned it, in the udp version we don't send buffer lengths. We send entire buffers, if they are too big and don't fit in the underlying udp packet, they will be truncated and the rest has to be disgarded (even from write_queue_size, which is not currently done correctly in my commit). So maybe adding a write_queue_count along the write_queue_size is kind of reasonable because it gives you some more information about how full the queue is.

My plan is now:

1. make uv__count_bufs internally available in the library as an inline function
2. implement write_queue_size and write_queue_count using uv__count_bufs. write_queue_count represents the number of sends, not the number of bufs given to send.
3. implement try_send
4. implement more efficient normal send

4 and 3 do not depend on each other, they both depend on 2 though which depends on 1.

[saghul]

Now that bnoordhuis mentioned it, in the udp version we don't send buffer lengths. We send entire buffers, if they are too big and don't fit in the underlying udp packet, they will be truncated and the rest has to be disgarded (even from write_queue_size, which is not currently done correctly in my commit). So maybe adding a write_queue_count along the write_queue_size is kind of reasonable because it gives you some more information about how full the queue is.

Not sure I follow. Can't we substract the length of the buffer and not the length of what was sent from write_queue_size?

[txdv]

@saghul This is exactly what I'm going to do and what I wanted to say.

However the write_queue_size is not a very useful metric for measuring how full the queue is, so I'm think adding a write_queue_count would be nice.

[ibc]

@txdv so great!

[saghul]

However the write_queue_size is not a very useful metric for measuring how full the queue is, so I'm think adding a write_queue_count would be nice.

I don't see why/where write_queue_count would help, can you provide an example use case? In TCP the queue size can be used to implement backoff, here not really, but gives the user the udea of how many bytes are pending to be sent, and he can stop sending for a bit until he sees that the level went down (by checking in send_cb) for example.

[txdv]

What useful information does write_queue_size give you if you add a buffer with a 64k buffer and a buffer with 1 byte? It will be 64k + 1, but is there anything you can reason about this number? You still have two sends, copying 64k takes probably a bit longer, but we are still crossing userspace/kernelspace which introduces somekind of overhead as well.

write_queue_count in this case gives us the value 2. 2 operations until the queue is freed.

[saghul]

What useful information does write_queue_size give you if you add a buffer with a 64k buffer and a buffer with 1 byte? It will be 64k + 1, but is there anything you can reason about this number? You still have two sends, copying 64k takes probably a bit longer, but we are still crossing userspace/kernelspace which introduces somekind of overhead as well.

Tells me how much data is in the queue exactly. After each write operation (in the callback) I can check how much is left.

write_queue_count in this case gives us the value 2. 2 operations until the queue is freed.

I'm -1 on this. We might change how writes are dealt with internally in the future and coalesce them, so the number of requests won't be equal to the number of operations.

[ibc]

@saghul please remember we are talking about UDP. You must send entire datagrams one by one, regardless how long each one is.

[txdv]

I'm -1 on this. We might change how writes are dealt with internally in the future and coalesce them, so the number of requests won't be equal to the number of operations.

They will be in udp. This is exactly the point.

[saghul]

Doh, you guys are right. Sorry, I'm doing multiple things at once. It's fine then.

[txdv]

First step: https://github.com/txdv/libuv/commit/ff10e6d6c8fbdd16ed8f3cfaaf1bd1f8d0a117bf

I have to check if it works on windows before a pull request.

It is strange that in WINDOWS you have to CAPSLOCK the INLINE keyword.

[saghul]

We might need some UV_INLINE helper macro, for MSVC.

[txdv]

According to this: http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx

The inline keyword is available only in C++. The inline and forceinline keywords are available in both C and C++. For compatibility with previous versions, _inline is a synonym for __inline.

Now we look into the code and yes, @saghul you are right.

Now the question is, where do we put the UV_INTERNAL?

[ibc]

@txdv humm, you are defining an inline function in uv-common.c, that is not good and will cause "undefined symbol" if you call that funcion from two or more different source files (which is true). And in uv-common.h you've added an inline declaration (with no body), which honestly I don't understand.

AFAIK inline functions must be declared and defined in header files. The only case in which you can declare a function as inline within a source file (.c) is when such a function is "private" and just called from the same source file.

Correct me if I'm wrong, please.

[saghul]

Now the question is, where do we put the UV_INTERNAL?

I'd put it in uv-common.

[saghul]

@ibc is right. uv-common.c will be compiled to uv-common.o, so no code sharing can happen unless you define the function on the h file. So, lets drop the inline, I don't think it'll matter.

[txdv]

@ibc You are right. I think this goes the same for C compilers. If this function is worthy inlining, the compiler will do it itself.

https://github.com/txdv/libuv/commit/8017c67b75f901741d330d7802ee49a8beb7465d now we don't need to rely on UV_INTERNAL anymore.

[ibc]

+1

[txdv]

I added and tested write_queue_size to/on both windows and unix versions: https://github.com/txdv/libuv/commits/uv__count_bufs

I copied the code from the tcp code base, so it should work...

Well, they lack a testcase itself, but .... Is it possible to add one? Create one socket which doesn't read and write into it with the other until something is queued? What do you think guys? Will you give me many thumb ups if I create some test cases?

[ibc]

The point is: what do you want to test it? right now you have not added new API yet so by using your libuv modified version (which has same API yet) I cannot figure how to test the new additions (may be I miss something).

I expect that the way to go is, once uv_udp_try_send() is added, code a test similar to test/test-tcp-try-write.c, but note that such a test DOES NOT require that uv_try_write() can write (or not) all the given buffer (it may happen or not, which depends on the host, CPU usage, etc).

[txdv]

we have write_queue_size on udp which we can test.

[ibc]

There are some stuff in current tests:

~/src/libuv/test/ $ ack write_queue_size

benchmark-tcp-write-batch.c
82:  ASSERT(req->handle->write_queue_size == 0);

test-tcp-writealot.c
70:  ASSERT(tcp->write_queue_size == 0);

[saghul]

What do you think guys? Will you give me many thumb ups if I create some test cases?

Major props for extra tests! Plus, first round on me if you ever find yourself in Amsterdam :-)

[txdv]

looks like I have to add an assert after each udp test to check if the write_queue_size is empty

[txdv]

I just added the tests.

I just piggy bagged them with the other tests.

https://github.com/txdv/libuv/tree/uv__count_bufs

@indutny @saghul I think this should be a self contained pull request. What do you think? Is adding just some asserts in the other testcases enough of a test?

I will add udp.write_queue_count now.

[saghul]

I would rather have it as a commit of the try_send PR. On Jun 29, 2014 5:49 PM, "Andrius Bentkus" notifications@github.com wrote:

I just added the tests.

I just piggy bagged them with the other tests.

https://github.com/txdv/libuv/tree/uv__count_bufs

@indutny https://github.com/indutny @saghul https://github.com/saghul I think this should be a self contained pull request. What do you think?

I will add udp.write_queue_count now.

— Reply to this email directly or view it on GitHub https://github.com/joyent/libuv/issues/1320#issuecomment-47458436.

[txdv]

Should I name the fields send_queue_size? and send_queue_count?

[ibc]

Yes, much better.

Iñaki Baz Castillo ibc@aliax.net On Jun 29, 2014 9:18 PM, "Andrius Bentkus" notifications@github.com wrote:

Should I name the fields send_queue_size? and send_queue_count?

— Reply to this email directly or view it on GitHub https://github.com/joyent/libuv/issues/1320#issuecomment-47469319.

[saghul]

Sounds good. On Jun 29, 2014 9:17 PM, "Andrius Bentkus" notifications@github.com wrote:

Should I name the fields send_queue_size? and send_queue_count?

— Reply to this email directly or view it on GitHub https://github.com/joyent/libuv/issues/1320#issuecomment-47469319.

[ibc]

Yep, write has a "streaming" meaning. On Jun 29, 2014 9:46 PM, "Saúl Ibarra Corretgé" notifications@github.com wrote:

Sounds good. On Jun 29, 2014 9:17 PM, "Andrius Bentkus" notifications@github.com wrote:

Should I name the fields send_queue_size? and send_queue_count?

— Reply to this email directly or view it on GitHub https://github.com/joyent/libuv/issues/1320#issuecomment-47469319.

— Reply to this email directly or view it on GitHub https://github.com/joyent/libuv/issues/1320#issuecomment-47471113.

[txdv]

That name confused @saghul earlier

[txdv]

Here is my initial pull request: https://github.com/joyent/libuv/pull/1344

Please move the discussion over there and review the code.