GNOME Bugzilla – Bug 719646
GSocket: add g_socket_send_messages() to send multiple messages in one go
Last modified: 2014-12-11 18:04:09 UTC
It would improve performance for some applications, such as streaming video over RTP, if GSocket had an API for sending multiple packets with sendmmsg().
Looks like a great use for an array or list of GBytes...
I will look into this.
Created attachment 278514 [details] [review] gio/tests/socket: add test for g_socket_send_message() This just adds some test code for existing API and is a prerequisite for later patches.
Created attachment 278515 [details] [review] gio/tests/socket: add datagram version of test_ip_sync This just adds some test code for existing API and is a prerequisite for later patches.
Created attachment 278516 [details] [review] gsocket: add g_socket_send_messages() Allows sending of multiple messages (packets, datagrams) in one go using sendmmsg(), thus drastically reducing the number of syscalls when sending out a lot of data, or when sending out the same data to multiple recipients. Very useful for high-bandwidth video streaming (MPEG-TS over UDP / RTP / WebRTC) and/or sending data to multiple recipients without multicast.
Created attachment 278517 [details] [review] gio/tests/socket: add unit test for g_socket_send_messages()
Created attachment 278983 [details] [review] gsocket: add g_socket_send_messages() [v2] Allows sending of multiple messages (packets, datagrams) in one go using sendmmsg(), thus drastically reducing the number of syscalls when sending out a lot of data, or when sending out the same data to multiple recipients. Changes to previous patch: - add bytes_sent field to GOutputMessage and fill it in. This matches struct mmsghdr and is needed when handling errors.
Created attachment 278985 [details] [review] gio/tests/socket: add unit test for g_socket_send_messages() [v2] Adds unit test, requires the first two gio/tests/socket patches. Changes to previous version of the patch: - generate an error adn test error behaviour - don't send message with empty vectors, the behaviour of what happens with that varies on the implementation used and is not a particularly useful thing to test anyway.
Review of attachment 278983 [details] [review]: Sucks about the new datatype.... Looks pretty good actually -- just one comment below. ::: gio/gsocket.c @@ +4208,3 @@ + /* we have to iterate over all messages here, because if an error occured + * we don't know where between num_sent and num_messages that happened */ + for (i = 0; i < num_messages; ++i) In the non-error case of num_sent > 0, but we got EWOULDBLOCK (and hit the break; above) then we are indeed in a very well-known state and we should only fill in up to num_sent, no?
Created attachment 289543 [details] [review] gsocket: small send_messages() optimisation for EWOULDBLOCK case If we have already sent some messages but then get EWOULDBLOCK when trying to send more, we know that we have sent exactly num_sent messages and thus don't have to go over all messages to copy back the number of bytes sent out in that case. --- This should address your latest comment if I understood it correctly. The max_sent assignment in the error code path is superfluous but I left it there for clarity to go along with the comment.
Review of attachment 289543 [details] [review]: These two patches really ought to be merged rather than have a bugfix to the original follow in a commit marked "optimisation". ::: gio/gsocket.c @@ +4198,3 @@ errsv == EAGAIN)) + { + max_sent = num_sent; I think you probably also want to set 'result =' here?
> These two patches really ought to be merged rather than have a bugfix to the > original follow in a commit marked "optimisation". Ack, will squash them. > ::: gio/gsocket.c > @@ +4198,3 @@ > errsv == EAGAIN)) > + { > + max_sent = num_sent; > > I think you probably also want to set 'result =' here? I don't think it's needed, since result is inited to 0 and updated at the end of the while() loop as we go along. Are you saying you think it's required or suggesting it should be done for clarity/style reasons here as well?
Created attachment 292489 [details] [review] gsocket: add g_socket_send_messages() [v3] Allows sending of multiple messages (packets, datagrams) in one go using sendmmsg(), thus drastically reducing the number of syscalls when sending out a lot of data, or when sending out the same data to multiple recipients. ----------------- Same as v2, but with attachment 289543 [details] [review] squashed into it, and GLIB_AVAILABLE_IN_2_42 -> GLIB_AVAILABLE_IN_2_44 in gio/gsocket.h.
Review of attachment 292489 [details] [review]: Indeed, you're right. Looks fine now. Thanks.
Review of attachment 278985 [details] [review]: okay
Review of attachment 278515 [details] [review]: okay
Review of attachment 278514 [details] [review]: okay
Pushed to master, many thanks for the reviews! commit ae1b6ecd9d1b5e0fa701cd2fd44425ab2312ab02 Author: Tim-Philipp Müller <tim@centricular.com> Date: Fri Jun 13 20:19:09 2014 +0100 gio/tests/socket: add unit test for g_socket_send_messages() https://bugzilla.gnome.org/show_bug.cgi?id=719646 commit fff5c7cd631f7eefeb93412b1d9d90517c4b895e Author: Tim-Philipp Müller <tim@centricular.com> Date: Thu Jun 12 18:16:45 2014 +0100 gsocket: add g_socket_send_messages() Allows sending of multiple messages (packets, datagrams) in one go using sendmmsg(), thus drastically reducing the number of syscalls when sending out a lot of data, or when sending out the same data to multiple recipients. https://bugzilla.gnome.org/show_bug.cgi?id=719646 commit 3c3fc0e463278f368e6192ff65a45b6873bf370a Author: Tim-Philipp Müller <tim@centricular.com> Date: Fri Jun 13 19:45:25 2014 +0100 gio/tests/socket: add datagram version of test_ip_sync commit 486485042719dd13c03d5261a7945e94bdf63d69 Author: Tim-Philipp Müller <tim@centricular.com> Date: Fri Jun 13 10:48:52 2014 +0100 gio/tests/socket: add test for g_socket_send_message()