Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luv_new_udp: Add support for UV_UDP_RECVMMSG #487

Merged
merged 3 commits into from
Oct 21, 2020

Conversation

squeek502
Copy link
Member

@squeek502 squeek502 commented Apr 22, 2020

See the added comment for my thought process here:

luv/src/udp.c

Lines 48 to 57 in 43fb118

// Libuv intended to enable this by default, but it caused a backwards-incompatibility with how
// the buffer is freed in udp_recv_cb, so it had to be put behind a flag to avoid breaking
// existing libuv users. However, because luv handles UV_UDP_MMSG_CHUNK in luv_udp_recv_cb, we can
// always enable this flag and get the benefits of recvmmsg for platforms that support it.
//
// Relevant links:
// - https://github.com/libuv/libuv/issues/2791
// - https://github.com/libuv/libuv/pull/2792
// - https://github.com/libuv/libuv/pull/2532
// - https://github.com/libuv/libuv/issues/419

Submitting this as a draft because there's still some more things to be figured out:

  • There are some bugs that need to be worked out with UV_UDP_RECVMMSG enabled, some of the udp tests are failing
  • I think the usefulness of recvmmsg depends on the buffer size, which we don't give Lua control of (we use the suggested_size from uv_alloc_cb). Maybe turning on UV_UDP_RECVMMSG only makes sense if we also allow changing the size of the recv buffer.
  • The parameter of luv_new_udp is a little bit strange now, I think we might want to allow it to be passed as a table and have different fields for address family and recvmmsg. This comment is relevant: Some minor API inconsistencies #399 (comment)

@squeek502 squeek502 marked this pull request as draft April 22, 2020 21:02
@squeek502
Copy link
Member Author

Some part of the test failures are from libuv bugs with UV_UDP_RECVMMSG. Upstream issue: libuv/libuv#2806

@squeek502
Copy link
Member Author

Valgrind/ASAN test failures might also be a libuv upstream issue: libuv/libuv#2818

@squeek502
Copy link
Member Author

More relevant info regarding

I think the usefulness of recvmmsg depends on the buffer size, which we don't give Lua control of (we use the suggested_size from uv_alloc_cb). Maybe turning on UV_UDP_RECVMMSG only makes sense if we also allow changing the size of the recv buffer.

from my comment here:

  • UV_UDP_RECVMMSG has no real effect unless the allocated buffer size is large enough to handle > 1 max size dgrams
  • When UV_UDP_RECVMMSG is used, the number of possible messages recv'd at once is limited to (buffer size / max dgram size).

suggested_size is equal to max dgram size, so without altering the allocated buffer size (or giving control of the buffer size to Lua), turning on UV_UDP_RECVMMSG in luv will have no effect.

@squeek502 squeek502 mentioned this pull request Oct 18, 2020
This makes it so that Lua doesn't get irrelevant callbacks when recv_cb is called by Libuv only to free the buffer used by recvmmsg. See libuv/libuv#2836 for more context
@squeek502
Copy link
Member Author

squeek502 commented Oct 18, 2020

Ok, this is ready for review. I decided against enabling recvmmsg by default and instead the user has to opt in by setting the number of messages that can be received at a time by calling e.g. uv.new_udp({mmsgs = 4}) (to be able to receive 4 messages at once). See the updated docs for new_udp and the comment added to where the default for mmsgs is set:

luv/src/udp.c

Lines 31 to 37 in 407f9c3

// TODO: This default can potentially be increased, but it's
// not clear what the best default would be, or if unconditionally
// using recvmmsg is always an improvement.
//
// Would probably need to do some extensive benchmarking to
// figure out what a good default might be.
int mmsg_num_msgs = 1;

(in some very basic testing, I noticed that using recvmmsg has some overhead so I figured always enabling it is probably not the correct decision; I don't know much about its use case, though, so if anyone has more experience let me know what you think)

All the rest is handled automatically, though, and there's no difference from the Lua side (the same number of Lua recv callbacks are called for a udp handle that is using recvmmsg as one that isn't).

@squeek502 squeek502 marked this pull request as ready for review October 18, 2020 19:54
@squeek502 squeek502 changed the title luv_new_udp: Always turn on UV_UDP_RECVMMSG luv_new_udp: Add support for UV_UDP_RECVMMSG Oct 18, 2020
@squeek502 squeek502 requested a review from zhaozg October 20, 2020 18:40
@squeek502 squeek502 merged commit 464b735 into luvit:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants