GNOME Bugzilla – Bug 743278
Avoid copying bytearrays when marshalling
Last modified: 2015-02-16 19:42:28 UTC
When marshalling from Python to C an unnecessary copy of the bytearray is made if the C function does take ownership of the bytearray (GI_TRANSFER_NOTHING). https://git.gnome.org/browse/pygobject/tree/gi/pygi-array.c#n235 Too bad we can't also fix the copy when marshaling from C to Python, although I guess we could expose it as a memoryview if a feature flag is set.
Created attachment 295069 [details] [review] Avoid copying bytearrays from Python to C when transfer nothing There is already a test that triggers this change.
Review of attachment 295069 [details] [review]: It's not immediately clear to me if the patch is safe. For instance, what if the callee stores a ref to the array, then later in Python, the original bytes object is collected? /* C */ void my_object_set_bytes (MyObject *obj, GByteArray *bytes) { obj->priv->bytes = g_byte_array_ref (bytes); } # Python some_bytes = b'foobar' myobj.set_bytes(some_bytes) del some_bytes See also bug #709976 ::: gi/pygi-array.c @@ +238,3 @@ + const gchar *data = PYGLIB_PyBytes_AsString (py_arg); + + if (arg_cache->transfer == GI_TRANSFER_NOTHING && I think this needs a comment explaining this is for zero-copy and how it works.
Created attachment 295355 [details] [review] Avoid copying bytearrays from Python to C when transfer nothing v2 (In reply to comment #2) > Review of attachment 295069 [details] [review]: > > It's not immediately clear to me if the patch is safe. For instance, what if > the callee stores a ref to the array, then later in Python, the original bytes > object is collected? > > /* C */ > void my_object_set_bytes (MyObject *obj, GByteArray *bytes) { > obj->priv->bytes = g_byte_array_ref (bytes); > } > > # Python > some_bytes = b'foobar' > myobj.set_bytes(some_bytes) > del some_bytes > > See also bug #709976 > This works because it is a bytearray in the C sense: /* C */ void my_object_set_bytes (MyObject *obj, const guint8 *bytes, gsize length) { obj->priv->bytes = g_bytes_new (bytes, length); } > ::: gi/pygi-array.c > @@ +238,3 @@ > + const gchar *data = PYGLIB_PyBytes_AsString (py_arg); > + > + if (arg_cache->transfer == GI_TRANSFER_NOTHING && > > I think this needs a comment explaining this is for zero-copy and how it works. I added a basic comment, but I think it was only confusing due to a mix up in terms. There really isn't any magic here, just avoiding a temporary copy.
I think GArray with an item type of uint8 as well as a GByteArray will also hit this code path (which is ugly). If this is the case, it might just be a matter of adding a check for GI_ARRAY_TYPE_C to the conditional. See the array_success tag for non c array args: https://git.gnome.org/browse/pygobject/tree/gi/pygi-array.c?id=3.14.0#n394
Created attachment 295568 [details] [review] Avoid copying bytearrays from Python to C when transfer nothing v3 Updated to fix issue and modified test to check that the PyBytes object is not being modified. Although it looks like any API taking a GBytesArray cannot modify it and have the Python object reflect the change because it is actually a PyBytes...
Created attachment 295569 [details] [review] Modify the GByteArray in tests_bytearray_none_in This is to check that the API can/cannot make modification to the binding's object. ---- I used this patch to verify that the prior patch was broken in this regard and that this fixed it. Don't know if this should actually go upstream as it could break other bindings using the API.
ping?
Review of attachment 295568 [details] [review]: LGTM, thanks.
This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.