Btw, it would also be good to benchmark raw message serialization against libdbus-1 and against a raw socket (say, compare pushing 10MiB bytes vs a D-Bus message with body type 'ay' and same 10MiB data). This should probably be done prior to doing this work.

Created attachment 214521 [details]
Benchmark.

I'm attaching a benchmark I wrote to time marshaling and serialization. Currently it can test marshaling/serialization, serialization (ie, converting a GDBusMessage to a blob), or deserialization.

Created attachment 214523 [details] [review]
Proposed patch for some low-hanging fruit.

This patch improves serialization and deserialization performance by something like 45% based on my testing. I've added classes, called GInputBuffer and GOutputBuffer, that have functionality similar to GDataInputStream and GDataOutputStream but are not GInputStream/GOutputStream implementations and assume that data will be written to/from memory (thus avoiding the need to do things like iterate in order to make sure that all of the data is written).

Review would be welcome.

(In reply to comment #3)

> I've added classes, called GInputBuffer and
> GOutputBuffer, that have functionality similar to GDataInputStream and
> GDataOutputStream but are not GInputStream/GOutputStream implementations and
> assume that data will be written to/from memory (thus avoiding the need to do
> things like iterate in order to make sure that all of the data is written).

Wait, GMemoryInput/OutputStream can make just the same assumption, no ?
If at all possible, I'd like to avoid yet more buffer/stream api variants...

(In reply to comment #4)
> 
> > I've added classes, called GInputBuffer and
> > GOutputBuffer, that have functionality similar to GDataInputStream and
> > GDataOutputStream but are not GInputStream/GOutputStream implementations and
> > assume that data will be written to/from memory (thus avoiding the need to do
> > things like iterate in order to make sure that all of the data is written).
> 
> Wait, GMemoryInput/OutputStream can make just the same assumption, no ?

If we added some methods to it (ie, similar to what we have for GDataInputStream/GDataOutputStream). I guess there would be some performance impact vs. my original classes, since there will be a pointer deference for private data, and we need to call functions to get/set
the file position, but I'm guessing that the former isn't that significant, and the latter isn't done that often. I'll try it and see how well it works out.

I've reworked this patch to add functions to GMemoryInputStream/GMemoryOutputStream to directly read/write data. This provides performance improvements close to those of my original patch when serializing gdbus messages, although the original patch is slightly more performant. Also, setting properties via g_object_new incurs significant overhead with the creation of gvalues, etc., and a GMemoryOutputStream is constructed any time a GDBusMessage is generated, so I'll attach a patch (separate from the other change) to have g_memory_output_stream_new set properties directly.

Created attachment 226757 [details] [review]
Patch to have g_memory_output_stream_new directly initialize properties.

The downside I can see to this is that, if the architecture of GMemoryOutputStream is modified in the future such that setting these properties is no longer done the same way, then g_memory_output_stream_new will need to be maintained in parallel, although it reduces its instruction count by something like 23% on my system, according to callgrind, and thus improves the speed of gdbus message serialization by something like 3% depending on the size of the message, so making the change seems worth it to me.

Created attachment 226758 [details] [review]
Patch to add methods to directly read/write data to/from memory streams.

Review of attachment 226758 [details] [review]:

Hmmm.   That's a lot of new symbols, with only one user as of yet, I assume.  What do you think about just having a private subset of this used just by GDBus?

I mean, to use these functions anyways, you're going to have to hack up gdbusmessage.c anyways, right?  It seems like we could just have a basic

struct MemBuf {
  guint8* buf;
  gsize len;
  gsize pos;
};

internally.

(In reply to comment #9)
> Review of attachment 226758 [details] [review]:
> 
> Hmmm.   That's a lot of new symbols, with only one user as of yet, I assume. 
> What do you think about just having a private subset of this used just by
> GDBus?

I'd thought that it might be generally useful to have a fast alternative to GDataOutputStream for code that writes a bunch of data to memory, since GDataOutputStream and GDataInputStream are really designed for cases where you need the abstraction. But, yeah, gdbus is the only use case that I know of right now, so putting the code inside gdbusmessage.c would be another option if people would rather not add a bunch of symbols (it would look a bit like my original patch but not be public).

> I mean, to use these functions anyways, you're going to have to hack up
> gdbusmessage.c anyways, right?  It seems like we could just have a basic
> 
> struct MemBuf {
>   guint8* buf;
>   gsize len;
>   gsize pos;
> };
> 
> internally.

(In reply to comment #10)
> But, yeah, gdbus is the only use case that I know of
> right now, so putting the code inside gdbusmessage.c would be another option if
> people would rather not add a bunch of symbols

I don't really love adding all that not-really-memory-stream-related API to GMemory{Input,Output}Stream, so this sounds best to me.

Created attachment 227768 [details] [review]
Updated patch.

This only touches gdbusmessage.c. It adds a private struct and functions to easily read from / write to it.

Review of attachment 227768 [details] [review]:

Thanks for doing the refactoring...ideally we would share this somewhere else, but I'm trying to follow the general rule of only making public API when we have two consumers.

I did some spot checks at various places, and the code looks right, besides one typo.  

The thing we should look out for first is any potential points where we're buffer-overflowing.  This code is security sensitive - system daemons will be ported to GDBus.  Secondarily, correctness.  I assume you've run "make check" with this and ideally logged into a gnome session?

::: gio/gdbusmessage.c
@@ +83,3 @@
+
+  res = MIN (count, mbuf->valid_len - mbuf->pos);
+  memcpy (buffer, mbuf->data + mbuf->pos, res);

This is fine as a first step that just does s/g_data_input_stream/g_memory_buffer/, but it's kind of lame to memcpy() here when we could just return a pointer into the buffer to the caller.

Any optimizations on top should be a separate patch I think.

@@ +1428,3 @@
+                   G_IO_ERROR_INVALID_ARGUMENT,
+                   g_dngettext (GETTEXT_PACKAGE,
+                                "Wanted to read %lu bytee but only got %lu",

bytes, not bytee

(In reply to comment #13)
> Review of attachment 227768 [details] [review]:
> 
> I did some spot checks at various places, and the code looks right, besides one
> typo.  
> 
> The thing we should look out for first is any potential points where we're
> buffer-overflowing.  This code is security sensitive - system daemons will be
> ported to GDBus.  Secondarily, correctness.  I assume you've run "make check"
> with this and ideally logged into a gnome session?

Make check passes; yes.

> ::: gio/gdbusmessage.c
> @@ +83,3 @@
> +
> +  res = MIN (count, mbuf->valid_len - mbuf->pos);
> +  memcpy (buffer, mbuf->data + mbuf->pos, res);
> 
> This is fine as a first step that just does
> s/g_data_input_stream/g_memory_buffer/, but it's kind of lame to memcpy() here
> when we could just return a pointer into the buffer to the caller.
> 
> Any optimizations on top should be a separate patch I think.
> 

Hmm. g_memory_buffer_read is only called by read_string, which allocates a buffer and then modifies it to add a null byte (which in theory is present in the blob being read). So I'd have to move the memcpy into read_string to keep things roughly as they were. I think that now I could simplify read_string to avoid the memcpy/malloc/free and then just remove g_memory_buffer_read, but such a change should be reviewed carefully, so, yeah, submitting it as a separate patch makes sense to me.

> @@ +1428,3 @@
> +                   G_IO_ERROR_INVALID_ARGUMENT,
> +                   g_dngettext (GETTEXT_PACKAGE,
> +                                "Wanted to read %lu bytee but only got %lu",
> 
> bytes, not bytee

Thanks. Fixed.

Created attachment 229919 [details] [review]
Revised patch.

Fixed a typo, and have g_memory_buffer_read return a pointer into the buffer rather than allocate memory (move the malloc into read_string for now).

Review of attachment 229919 [details] [review]:

Ok, I did another review pass, and caught some things I hadn't noticed before.

Please commit though after addressing the below:

::: gio/gdbusmessage.c
@@ +201,3 @@
+    {
+    case G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN:
+      v = GINT32_FROM_BE (v);

Should be GUINT32_FROM_BE

@@ +204,3 @@
+      break;
+    case G_DATA_STREAM_BYTE_ORDER_LITTLE_ENDIAN:
+      v = GINT32_FROM_LE (v);

Ditto

@@ +259,3 @@
+    {
+    case G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN:
+      v = GINT64_FROM_BE (v);

GUINT64_FROM_BE

@@ +262,3 @@
+      break;
+    case G_DATA_STREAM_BYTE_ORDER_LITTLE_ENDIAN:
+      v = GINT64_FROM_LE (v);

Ditto

@@ +298,3 @@
+  data = g_realloc (mbuf->data, size);
+
+  if (size > 0 && !data)

This can't happen since g_realloc() won't return NULL ever.  Possibly you meant g_try_realloc() ?  But it's not totally clear to me what this "allow_partial" thing is about.  This function is only ever called with the parameter set to TRUE.

@@ +393,3 @@
+    }
+  
+  return g_memory_buffer_write (mbuf, &data, 2);

g_memory_buffer_write() returns gssize, but these functions are returning gboolean; an error from _write() as -1 will be treated as success.

I'd change g_memory_buffer_write() to return a gboolean success value, since no caller appears to care about short writes.

(In reply to comment #16)
> Review of attachment 229919 [details] [review]:
> 
> @@ +298,3 @@
> +  data = g_realloc (mbuf->data, size);
> +
> +  if (size > 0 && !data)
> 
> This can't happen since g_realloc() won't return NULL ever.  Possibly you meant
> g_try_realloc() ?  But it's not totally clear to me what this "allow_partial"
> thing is about.  This function is only ever called with the parameter set to
> TRUE.

I adapted the code from the more generic GMemoryOutputStream, which allows the user to specify a realloc function that could potentially fail without aborting, so that code is left over from it. The original gdbus code uses g_realloc, so I'm just going to remove the code that is there to handle realloc failing.

Comment on attachment 229919 [details] [review]
Revised patch.

Attachment 229919 [details] committed to master, with some modifications (3e5214).

Created attachment 230305 [details] [review]
Patch to optimize reading strings.

Now that we directly access the memory containing the message blob, and since strings are expected to be terminated with a null byte anyhow, we can just access the string directly from the blob; we don't need to dynamically allocate a buffer and copy the string. This improves the speed of read_string by more than half, going by callgrind's output.

I believe this is functionally equivalent to the current code, but nevertheless it needs to be reviewed...

Review of attachment 230305 [details] [review]:

One comment to fix in this patch - two others for future patches.

::: gio/gdbusmessage.c
@@ +1374,1 @@
+  if (mbuf->pos + len >= mbuf->valid_len || mbuf->pos + len < mbuf->pos)

While doing some more looking at this code, I can't find any part of GDBus that verifies that the total length it's parsing is less than the max message size.  

There is an assertion that the *claimed* size from the header is < (2<<27), but no actual check of the total bytes read is less than that.

Not something to fix in this patch, just looking at boundary conditions.

@@ +1393,3 @@
+      str = g_malloc (len + 1);
+      memcpy (str, mbuf->data + mbuf->pos, len);
+      str[len] = '0';

These 3 lines are just: 

 str = g_strndup (mbuf->data + mbuf->pos, len);

@@ +1596,3 @@
           if (v == NULL)
             goto fail;
           ret = g_variant_new_string (v);

Right so now, you could get to zero copy if instead of returning a "const gchar *" from read_string(), you returned a new GBytes instance that held a ref on the memory buffer, and then do:

ret = g_variant_new_from_bytes ("s", buf);

Just follow the chain of functions that g_variant_new_string() calls to see why this works.

The downside of this is that anyone holding a ref on the resulting variant is actually holding a ref to the entire raw DBus message.  But I think in a lot of cases, that's totally fine.  If your message just holds one string, it's not much overhead.  If you have a message that's 2 strings, an a{sv} structure with even more strings, it looks like a lot more of a win I imagine.

(In reply to comment #20)
> The downside of this is that anyone holding a ref on the resulting variant is
> actually holding a ref to the entire raw DBus message.  But I think in a lot of
> cases, that's totally fine.  If your message just holds one string, it's not
> much overhead.  If you have a message that's 2 strings, an a{sv} structure with
> even more strings, it looks like a lot more of a win I imagine.

I think a change like this is interesting, but would it in a separate commit, separate from less other, less invasive optimizations. Thanks!

Comment on attachment 230305 [details] [review]
Patch to optimize reading strings.

Changed the malloc/memcpy to a g_strdup0 and committed (fe0b77). Thanks for the review.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/418.