GNOME Bugzilla – Bug 732754
GDBusMessage: optimise (de)serialisation of fixed arrays
Last modified: 2014-07-24 13:51:57 UTC
While trying to reproduce bug 732737 I decided to store a 50MB string in dconf. Due to the missing features in the D-Bus serialisation format, dconf sends all messages to the service as a GVariant-formatted blob of 'ay'. This means that I was sending a blob of ~50MB. Unfortunately, GDBus handles this by creating 50 million separate one-byte GVariant instances.... Let's fix that.
Created attachment 279933 [details] [review] GDBusMessage: simplify byteswapping Remove some duplication by simplifying our byteswap checks.
Created attachment 279934 [details] [review] GDBusMessage: fast-path decoding of fixed arrays Instead of creating a separate GVariant for each of the 'y's in an 'ay', use g_variant_new_fixed_array().
Created attachment 279935 [details] [review] GDBusMessage: fast-path encoding of fixed arrays Instead of handling each item separately, handle the array as a whole.
Review of attachment 279934 [details] [review]: ::: gio/gdbusmessage.c @@ +1208,2 @@ static gboolean +get_type_fixed_size (const GVariantType *type) You return 0/1/2/4/8 not TRUE/FALSE. So make this return guint or gsize not gboolean. @@ +1641,2 @@ const GVariantType *element_type; + guint32 fixed_size; In the following patch you use guint... pick one? :-)
Review of attachment 279933 [details] [review]: ::: gio/gdbusmessage.c @@ +71,3 @@ + return mbuf->byte_order == G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN; +#else + return byte_order == G_DATA_STREAM_BYTE_ORDER_LITTLE_ENDIAN; mbuf->byte_order
(In reply to comment #4) > Review of attachment 279934 [details] [review]: > > ::: gio/gdbusmessage.c > @@ +1208,2 @@ > static gboolean > +get_type_fixed_size (const GVariantType *type) > > You return 0/1/2/4/8 not TRUE/FALSE. So make this return guint or gsize not > gboolean. This was get_type_is_fixed_size() before, then I changed it. Oops :) > > @@ +1641,2 @@ > const GVariantType *element_type; > + guint32 fixed_size; > > In the following patch you use guint... pick one? :-) sure. Will redo. (In reply to comment #5) > Review of attachment 279933 [details] [review]: > mbuf->byte_order thx.
Created attachment 279972 [details] [review] GDBusMessage: simplify byteswapping Remove some duplication by simplifying our byteswap checks.
Created attachment 279973 [details] [review] GDBusMessage: fast-path decoding of fixed arrays Instead of creating a separate GVariant for each of the 'y's in an 'ay', use g_variant_new_fixed_array().
Review of attachment 279973 [details] [review]: looks reasonable. I assume we have test coverage for this path ?
Review of attachment 279972 [details] [review]: sure, ok
(In reply to comment #9) > Review of attachment 279973 [details] [review]: > > looks reasonable. I assume we have test coverage for this path ? we do. gdbus-serialization actually caught me when I failed to include the byteswap...
Comment on attachment 279972 [details] [review] GDBusMessage: simplify byteswapping Attachment 279972 [details] pushed as d108ada - GDBusMessage: simplify byteswapping
Review of attachment 279973 [details] [review]: ok then
Review of attachment 279973 [details] [review]: Holy shit, how did we ship with this for so long... ::: gio/gdbusmessage.c @@ +1407,3 @@ + gconstpointer result; + + if (mbuf->pos + len > mbuf->valid_len || mbuf->pos + len < mbuf->pos) G_UNLIKELY? @@ +1902,2 @@ if (ret != NULL) + g_variant_take_ref (ret); Very minor, but would prefer this as a separate patch as it's semantically unrelated.
Attachment 279935 [details] pushed as 958da1e - GDBusMessage: fast-path encoding of fixed arrays Attachment 279973 [details] pushed as 5463c8c - GDBusMessage: fast-path decoding of fixed arrays
(In reply to comment #14) > Very minor, but would prefer this as a separate patch as it's semantically > unrelated. Sorry that I didn't see this before the push :( I had to make that change because of the use of g_variant_byteswap(). This returns a hard reference, not floating, which means that this had to change to g_variant_take_ref(). As for the UNLIKELY, it was a copy/paste from a similar function for handling strings, but sure -- I can do another patch... for both cases.
Created attachment 281404 [details] Test case for regression This broke encoding of an array of doubles. Attached is a rough test.
Downstream regression: https://bugzilla.redhat.com/show_bug.cgi?id=1122128
Created attachment 281417 [details] [review] gio: Add regression test for double array encoding
Created attachment 281435 [details] [review] gio: Fix regression encoding an array of doubles Take the simple slow path in this case. Encoding a double takes special precautions as you can see in append_value_to_blob() and friends.
Review of attachment 281435 [details] [review]: Please commit this only with a comment mentioning that we intentionally exclude 'd' and linking to the report.
Review of attachment 281417 [details] [review]: Would prefer this in one of the many existing files (like maybe the gdbus-serialization test) but will not block you if you do not want to take the effort to do that...
Review of attachment 281417 [details] [review]: Put the test in gdbus-serialization.c
Review of attachment 281435 [details] [review]: Added comment.
But *why* is it broken? Is the GVariant encoding of doubles different from DBus? Something to do with alignment? dbus_type_is_fixed() definitely returns TRUE for DBUS_TYPE_DOUBLE.
(In reply to comment #25) > But *why* is it broken? Is the GVariant encoding of doubles different from > DBus? > > Something to do with alignment? Ryan probably knows better, but see https://git.gnome.org/browse/glib/tree/gio/gdbusmessage.c#n2309
Created attachment 281566 [details] [review] Properly fix encoding of double arrays It turns out that this bug actually would (sometimes) impact any sort of fixed-sized array with an alignment requirement of 8 due to incorrectly counting the alignment inserted between the (aligned 4) array length and the actual data. Fix this properly and remove the exception for doubles.
Review of attachment 281566 [details] [review]: Seems correct to me.
Ok, and that bug would cause us to write too long of an array length. Thanks a lot for fixing this!