(Hmm, did you set the ancient 2.25 version on purpose for this report?)

(In reply to comment #1)
> (Hmm, did you set the ancient 2.25 version on purpose for this report?)

No, that's what was chosen by bugzilla or my browser or whatever. I'm using latest GIT master

(In reply to comment #0)
> Usually you don't receive from a new sender every packet so a possible solution
> would be to add caching for the GSocketAddresses in GSocket

Or in g_socket_address_new_from_native()

Sebastian, please file networking-related bugs against the "network" component, not "gio"

I'm actually hitting this in libnice, the object creation was actually using 30% of my CPU (while sending 25MB/s in ~1500 bytes packets). So this is important enough that we're considering fishing out the fd and using recvfrom() directly instead.

<tpm> (...) https://bugzilla.gnome.org/show_bug.cgi?id=668842 (...) is a huge problem for us right now
<danw> tpm: ah, yeah, i started poking at that once, and then realized i needed to make it thread-safe, and then went off and did something else
<danw> i'm happy to take a patch for it
<tpm> ah, cool
<tpm> do you have a preference how/where to start poking?
<danw> tpm: well, like i said, i think there's an argument for putting it in g_socket_address_new_from_native(), but otoh, if you put it into GSocket instead then you don't need to worry about threads. And no one has complained about this problem in connection with any APIs other than g_socket_receive_from()...
<tpm> e.g. just make it internal to the socket, or expose it or make it global or ..
<danw> make it internal to the socket
<tpm> alright, thanks
<danw> and per-socket
<danw> if per-socket doesn't work, then i'd say put it in g_socket_address_new_from_native() using a single global mutex-protected cache
<tpm> great (..)

For APIs that use GSocket internally but that don't use GSocketAddress in their API, we hit the same problem on the send side (in libnice, we have our own cache because of that).

Created attachment 228578 [details] [review]
Add caching for the receiver addresses for g_socket_receive_from()

In my testcase this is improving the performance by 50%:

gst-launch-1.0 fakesrc sizetype=fixed sizemax=188 ! udpsink port=5123
time gst-launch-1.0 udpsrc uri=udp://127.0.0.1:5123 ! fakesink num-buffers=500000

Goes from
real	0m12.198s
user	0m10.233s
sys	0m1.868s
to
real	0m5.710s
user	0m3.024s
sys	0m2.192s


The explicit storage of the native socket address inside the cache is done because calling g_socket_address_to_native() shows up with ~23% of instructions inside g_socket_address_receive_from() in callgrind.

Out of curiosity, what numbers do you get with the native API (i.e. 0.10) ?

real	0m4.856s
user	0m3.988s
sys	0m0.544s

Comment on attachment 228578 [details] [review]
Add caching for the receiver addresses for g_socket_receive_from()

>+/* Size of the receiver cache for g_socket_receive_from() */
>+#define RECV_ADDR_CACHE_SIZE 8

Is that based on anything in particular? Did you test other sizes?

>+    gpointer naddr;
>+    gint naddrlen;

I'd prefer

  struct sockaddr *native;
  gsize native_len;

(That applies pretty much everywhere you currently have "naddr".)

>+static GSocketAddress *
>+cache_recv_address (GSocket * socket, gpointer addr, int addrlen)

"GSocket *socket" (no space after "*"). (and again struct sockaddr * and gsize).

(I don't love the function name, but I don't have any especially better suggestions.)

>+  if (addrlen > 0)

Just do

    if (addrlen <= 0)
      return NULL;

and then you don't need to indent the rest of the function an extra level

>+          if (memcmp (tmp_naddr, addr, addrlen) == 0)
>+            {
>+              saddr = g_object_ref (tmp);
>+              socket->priv->recv_addr_cache[i].last_used = g_get_monotonic_time ();
>+              break;
>+            }

likewise, you can just "return saddr" instead of breaking here, and then you don't need the "if (!saddr)" below

>+          for (i = 0; i < RECV_ADDR_CACHE_SIZE; i++)
>+            {
>+              if (socket->priv->recv_addr_cache[i].last_used < oldest_time)

you could merge this into the previous loop so you're only iterating the cache once.

(In reply to comment #12)
> (From update of attachment 228578 [details] [review])
> >+/* Size of the receiver cache for g_socket_receive_from() */
> >+#define RECV_ADDR_CACHE_SIZE 8
> 
> Is that based on anything in particular? Did you test other sizes?

I didn't test any other sizes and had no scientific reason for chosing 8, just a feeling ;)

> >+    gpointer naddr;
> >+    gint naddrlen;
> 
> I'd prefer
> 
>   struct sockaddr *native;
>   gsize native_len;
> 
> (That applies pretty much everywhere you currently have "naddr".)

Alright, will change that later/tomorrow... and also all the other comments. This was just a proof of concept first so I didn't spend that much time in cleaning up the code before getting more comments about it going in the right direction.

Created attachment 228594 [details] [review]
Add caching for the receiver addresses for g_socket_receive_from()

Comment on attachment 228594 [details] [review]
Add caching for the receiver addresses for g_socket_receive_from()

just one tiny nit:

>+cache_recv_address (GSocket *socket, struct sockaddr * native, int native_len)

no space after the second "*"

otherwise ok to commit

Thanks for the fast reviews :) Is there a indent script or something for the GLib coding style?


commit 2bba1da30674686960571603961e8daed973e5d0
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Nov 9 15:28:36 2012 +0100

    Add caching for the receiver addresses for g_socket_receive_from()
    
    https://bugzilla.gnome.org/show_bug.cgi?id=668842