GNOME Bugzilla – Bug 773504
Avoid extra allocations
Last modified: 2017-07-10 14:21:49 UTC
During generation quite a few GStrings are allocated as is the list of members in an object. These can all be avoided and when testing a large JSON object the number of allocations was reduced in half. It would also be nice to allow dumping the JSON to a GString when other data is required, i.e. a header for JSON-RPC.
Created attachment 338466 [details] [review] Avoid multiple buffer allocations Instead share a single GString in all dump functions.
Created attachment 338467 [details] [review] object: Use a GQueue for members_ordered This makes the list always ordered and removes the g_list_reverse() in json_object_get_members().
Created attachment 338468 [details] [review] core: Avoid json_object_get_members() Use JsonObject's private members_ordered GQueue instead. This avoids a g_list_copy().
Created attachment 338469 [details] [review] generator: Add an GString-based method This allows callers to avoid extra allocations when dumping to a preexisting string.
Review of attachment 338467 [details] [review]: I'm not a maintainer, but just noticed this minor detail in json_object_foreach_member(): for (l = object->members_ordered.tail; l != NULL; l = l->prev) Should presumably be: for (l = object->members_ordered.head; l != NULL; l = l->next) As the old code was walking the list in the reverse direction because the list was stored in reverse, but the new code no longer stores it in reverse, it should walk front to back.
Review of attachment 338466 [details] [review]: Looks nice, thanks.
Review of attachment 338467 [details] [review]: Indeed, Ole André is correct. ::: json-glib/json-object.c @@ +889,3 @@ g_return_if_fail (func != NULL); + for (l = object->members_ordered.tail; l != NULL; l = l->prev) Now that `members_ordered` is stored in the right order, there's no need to traverse the list in reverse.
Review of attachment 338468 [details] [review]: Mmh, I don't particularly like accessing the contents of JsonObject directly. Maybe have a `json_object_get_members_internal()` function that returned a pointer to the GQueue?
Review of attachment 338469 [details] [review]: ::: json-glib/json-generator.h @@ +104,3 @@ +JSON_AVAILABLE_IN_1_4 +GString *json_generator_to_string (JsonGenerator *generator, I'd call it `to_gstring()` or `to_buffer()` instead. "String" is a bit of an overloaded term.
Review of attachment 338469 [details] [review]: Could you split the chunk that added the new version macros into its own commit?
Comment on attachment 338466 [details] [review] Avoid multiple buffer allocations Pushed attachment 338466 [details] [review] with a slight change to avoid compiler warnings about unused variables
Created attachment 349862 [details] [review] object: Use a GQueue for members_ordered v2 (In reply to Emmanuele Bassi (:ebassi) from comment #7) > Review of attachment 338467 [details] [review] [review]: > > Indeed, Ole André is correct. > > ::: json-glib/json-object.c > @@ +889,3 @@ > g_return_if_fail (func != NULL); > > + for (l = object->members_ordered.tail; l != NULL; l = l->prev) > > Now that `members_ordered` is stored in the right order, there's no need to > traverse the list in reverse. Fixed.
Created attachment 349863 [details] [review] core: Avoid json_object_get_members() v2 (In reply to Emmanuele Bassi (:ebassi) from comment #8) > Review of attachment 338468 [details] [review] [review]: > > Mmh, I don't particularly like accessing the contents of JsonObject directly. > > Maybe have a `json_object_get_members_internal()` function that returned a > pointer to the GQueue? Done.
Created attachment 349864 [details] [review] generator: Add an GString-based method (In reply to Emmanuele Bassi (:ebassi) from comment #9) > Review of attachment 338469 [details] [review] [review]: > > ::: json-glib/json-generator.h > @@ +104,3 @@ > > +JSON_AVAILABLE_IN_1_4 > +GString *json_generator_to_string (JsonGenerator *generator, > > I'd call it `to_gstring()` or `to_buffer()` instead. "String" is a bit of an > overloaded term. Done.
Review of attachment 349862 [details] [review]: Looks good.
Review of attachment 349863 [details] [review]: CR pass. Minor style nitpick, though; could you please fix it before pushing? ::: json-glib/json-types-private.h @@ +125,3 @@ +G_GNUC_INTERNAL +GQueue * json_object_get_members_internal No real need to go to a new line for the arguments; it's a private header, so you don't even need to re-align everything.
Review of attachment 349864 [details] [review]: ::: json-glib/json-generator.c @@ +481,3 @@ + * json_generator_to_gstring: (skip) + * @generator: a #JsonGenerator + * @string: (allow-none): a #GString, or %NULL The `allow-none` annotation is deprecated. In this case, it would be `nullable`. The memory management semantics of this function are a bit weird, though; I'm not entirely sure it's going to be usable by language bindings. @@ +489,3 @@ + * is allocated and used. + * + * Generates a JSON data stream from @generator This is what I'm worried about. This is a `transfer full` value if string is NULL, but a `transfer none` if string is not NULL. I don't think we want to be extra-clever, here. I'd always require a GString instance, and make this a `transfer none`. This way you can still write compact lines like: GString *buf = json_generator_to_gstring (generator, g_string_new ("")); without weird memory management semantics. It's, of course, entirely possible to say that this function is not meant for language bindings — in which case, we should use the `skip` annotation; but I'd rather avoid that. @@ +507,3 @@ + + root = generator->priv->root; + * Since: 1.4 Coding style nitpick: pointer comparisons should be against NULL — like you did above for `string`.
Created attachment 353927 [details] [review] core: Avoid json_object_get_members() v3
Created attachment 353928 [details] [review] generator: Add an GString-based method v3
Review of attachment 353927 [details] [review]: Looks good, thanks!
Review of attachment 353928 [details] [review]: Looks good, thanks!