After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 719646 - GSocket: add g_socket_send_messages() to send multiple messages in one go
GSocket: add g_socket_send_messages() to send multiple messages in one go
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Tim-Philipp Müller
gtkdev
Depends on:
Blocks: 732152
 
 
Reported: 2013-12-01 15:19 UTC by Jonas Holmberg
Modified: 2014-12-11 18:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gio/tests/socket: add test for g_socket_send_message() (1.73 KB, patch)
2014-06-16 08:41 UTC, Tim-Philipp Müller
committed Details | Review
gio/tests/socket: add datagram version of test_ip_sync (6.88 KB, patch)
2014-06-16 08:42 UTC, Tim-Philipp Müller
committed Details | Review
gsocket: add g_socket_send_messages() (13.22 KB, patch)
2014-06-16 08:44 UTC, Tim-Philipp Müller
none Details | Review
gio/tests/socket: add unit test for g_socket_send_messages() (2.21 KB, patch)
2014-06-16 08:46 UTC, Tim-Philipp Müller
none Details | Review
gsocket: add g_socket_send_messages() [v2] (13.48 KB, patch)
2014-06-23 10:40 UTC, Tim-Philipp Müller
reviewed Details | Review
gio/tests/socket: add unit test for g_socket_send_messages() [v2] (3.00 KB, patch)
2014-06-23 11:05 UTC, Tim-Philipp Müller
committed Details | Review
gsocket: small send_messages() optimisation for EWOULDBLOCK case (2.38 KB, patch)
2014-10-28 20:03 UTC, Tim-Philipp Müller
needs-work Details | Review
gsocket: add g_socket_send_messages() [v3] (13.63 KB, patch)
2014-12-10 22:13 UTC, Tim-Philipp Müller
committed Details | Review

Description Jonas Holmberg 2013-12-01 15:19:59 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().
Comment 1 Allison Karlitskaya (desrt) 2013-12-01 20:13:34 UTC
Looks like a great use for an array or list of GBytes...
Comment 2 Tim-Philipp Müller 2014-05-05 10:38:54 UTC
I will look into this.
Comment 3 Tim-Philipp Müller 2014-06-16 08:41:16 UTC
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.
Comment 4 Tim-Philipp Müller 2014-06-16 08:42:08 UTC
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.
Comment 5 Tim-Philipp Müller 2014-06-16 08:44:58 UTC
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.
Comment 6 Tim-Philipp Müller 2014-06-16 08:46:16 UTC
Created attachment 278517 [details] [review]
gio/tests/socket: add unit test for g_socket_send_messages()
Comment 7 Tim-Philipp Müller 2014-06-23 10:40:51 UTC
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.
Comment 8 Tim-Philipp Müller 2014-06-23 11:05:11 UTC
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.
Comment 9 Allison Karlitskaya (desrt) 2014-10-15 10:09:57 UTC
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?
Comment 10 Tim-Philipp Müller 2014-10-28 20:03:16 UTC
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.
Comment 11 Allison Karlitskaya (desrt) 2014-12-10 21:23:19 UTC
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?
Comment 12 Tim-Philipp Müller 2014-12-10 22:07:36 UTC
> 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?
Comment 13 Tim-Philipp Müller 2014-12-10 22:13:25 UTC
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.
Comment 14 Allison Karlitskaya (desrt) 2014-12-10 22:43:02 UTC
Review of attachment 292489 [details] [review]:

Indeed, you're right.  Looks fine now.  Thanks.
Comment 15 Allison Karlitskaya (desrt) 2014-12-10 22:43:26 UTC
Review of attachment 278985 [details] [review]:

okay
Comment 16 Allison Karlitskaya (desrt) 2014-12-10 22:43:35 UTC
Review of attachment 278515 [details] [review]:

okay
Comment 17 Allison Karlitskaya (desrt) 2014-12-10 22:43:46 UTC
Review of attachment 278514 [details] [review]:

okay
Comment 18 Tim-Philipp Müller 2014-12-11 15:25:09 UTC
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()