GNOME Bugzilla – Bug 685431
More complete GBytes wrappers
Last modified: 2012-11-20 01:48:49 UTC
This set of patches expands ByteArray to include two "backends" for storage: GByteArray for mutable storage, and GBytes for immutable storage. In the best case (and probably the most likely case), where we're simply shoving data around, e.g. getting data from C and then simply taking that and putting it back into C, we prevent a large round-trip copy, which is a huge win.
Created attachment 225702 [details] [review] byteArray: Remove JSCLASS_CONSTRUCT_PROTOTYPE
Created attachment 225703 [details] [review] byteArray: Provide native wrappers for bytes types
Created attachment 225704 [details] [review] arg: Put GError specialization in with the other special cases
Created attachment 225706 [details] [review] y# Attachment to Bug 685431 - More complete GBytes wrappers byteArray: Provide native wrappers for bytes types Changed a few things to respect the priv->array == NULL && priv->bytes == NULL case, meaning "". Not sure what to do wrt. get_byte_array when we have a GBytes. Previously, I constructed a GByteArray, leaving priv->bytes in place, which is terrible. Now, I convert the entire thing to a mutable array, but I'm curious if constructing and then freeing a temp GByteArray to pass to a function that needs it might be the better approach. We'd have to do some profiling and look at functions in The Real World that take a GByteArray. Giovanni, you're more familiar with this than I.
Review of attachment 225702 [details] [review]: I think I had this in the big "don't use JSCLASS_CONSTRUCT_PROTOTYPE" in bug 679688, but ok, let's do this.
Review of attachment 225702 [details] [review]: Pushed a more complete patch, like the one you had in that bug (are you ever going to update the patches in there?); git-bz didn't work.
Created attachment 225713 [details] [review] byteArray: Provide native wrappers for bytes types Attaching a new patch to fix the weird patch description issue.
Review of attachment 225713 [details] [review]: The patch looks fine (provided tests are passing), and finally makes GBytes usable beyond a opaque container of bytes. There is still the problem that ByteArrays don't take a fast path when converting to a C array of uint8, but that's a separate issue. Also, this breaks API for existing GBytes users, so 3.7.1. (In 3.7 we should really look at typed arrays if possible, or at least providing an interface compatible to them, so we can transition in the future)
Review of attachment 225704 [details] [review]: ::: gi/arg.c @@ +1438,3 @@ JSVAL_TO_OBJECT(value)); + } else if (g_type_is_a(gtype, G_TYPE_ERROR)) { + arg->v_pointer = gjs_gerror_from_error(context, Seems you have lost a gjs_typecheck_gerror here.
Hmm...it's definitely problematic to break API like this. We're now suddenly returning arrays where we returned GBytes instances, right? I think I'd rather go with custom binding API for GBytes instances. So for example, we add a custom toArray() method. The most useful thing for me though would be an API to construct GBytes from JS arrays (and ideally strings).
Created attachment 228202 [details] [review] Add new GBytes API and conversions First, change the internal byteArray class so that it supports holding a GBytes *or* a GByteArray. It's used as a container as well as a bridge from JS -> native conversions for the overrides code. Based on this: * Add new GLib.Bytes.newFromNative() that accepts strings or arrays * Allow passing a GBytes for a native API that takes a guint8*+length pair * Add toArray() method on GBytes boxed to convert it to a native array. The executive summary is that GLib.Bytes is not auto-converted; users can pass them around unmodified, and create instances of them directly. Modifying a GBytes instance requires an explicit toArray() conversion.
Review of attachment 228202 [details] [review]: I must say, I liked the idea of accessing read-only bytes as indexes on the GBytes object, without requiring a conversion to GByteArray (which might require a copy). Also, to me bytes.toArray() looks like it returns a new ByteArray object, instead of modifing the object in place. ::: modules/overrides/GLib.js @@ +278,3 @@ + else + tmpArray = ByteArray.fromArray(obj); + return tmpArray.toGBytes(); If GBytes is special-cased in argument marshalling, can't you do GLib.Bytes.new() here, and take the usual path for (array) (element-type uint8)?
(In reply to comment #12) > Review of attachment 228202 [details] [review]: > > I must say, I liked the idea of accessing read-only bytes as indexes on the > GBytes object, Well...I think for this type of thing, toArray() should be fine, because: > without requiring a conversion to GByteArray (which might > require a copy). We won't copy in that case, when you call toArray() the underlying byteArray class will just be a proxy for the GBytes. > Also, to me bytes.toArray() looks like it returns a new ByteArray object, > instead of modifing the object in place. Not sure what you mean. Note a tricky thing about this is that the byteArray is copy-on-write. So it is legal to modify the result of somebytes.toArray(). I need to add tests for this. > If GBytes is special-cased in argument marshalling, can't you do > GLib.Bytes.new() here, and take the usual path for (array) (element-type > uint8)? It's not special cased right now...I thought about auto-converting from strings/arrays to GBytes, but on the other hand, it's not too hard to call GLib.Bytes.fromNative(). I think we want to encourage a model where the user has to think about copying data around. With the explicit conversion, you can keep a GBytes around and use it multiple times relatively efficiently. But I dunno. I could be convinced to add an implicit conversion too.
Created attachment 228658 [details] [review] Add new GBytes API and conversions There are now annotations for GBytes in glib master, so we don't need the ugly newFromNative function.
Review of attachment 228658 [details] [review]: Your patch doesn't apply, and after manual merging it doesn't build because byte_array_ensure_initialized is missing. From a blind review, I can say the following: - "new GLib.Bytes()" won't work, on needs "GLib.Bytes.new()" - despite what's advertised, passing a GLib.Bytes in place of a GByteArray or guint8* doesn't work, one needs an explicit toArray(). While requiring it for mutability may be ok, I don't like it much just for passing bytes around. - despite what's advertised, passing a ByteArray in place of a guint8*+length works but takes a copy. To avoid that, you need to take the fastpath in to_explicit_array_internal() instead, as arrays with length sidestep gjs_value_to_g_argument(). - passing an Array in place of GBytes doesn't work, one needs an explicit GLib.Bytes.new, which involves a copy. This in particular affects GVariant's autopacking, as it cannot just shove the value down to g_variant_new_from_bytes(). - GLib.Bytes.new() and new GLib.Bytes() don't recognize if the passed in value is a ByteArray, so they may incur an unneeded copy.
In this discussion, let's use these terms: - "GBytes" and "GByteArray" to refer to the GLib data types - "byteArray" to mean gjs' internal byteArray.c file - "JS array" to refer to plain JS arrays - "typed JS array" to refer to the JavaScript typed arrays - "GBytes overrides" to refer to the GBytes bits in modules/overrides/GLib.js (In reply to comment #15) > Review of attachment 228658 [details] [review]: > > Your patch doesn't apply, and after manual merging it doesn't build because > byte_array_ensure_initialized is missing. Ah, depends on a prerequisite patch. Will attach. > From a blind review, I can say the following: > - "new GLib.Bytes()" won't work, on needs "GLib.Bytes.new()" This is the similar to GLib.Variant.new. Dunno. This goes back to the questions about boxed types constructors and trying to make them more automatic by recognizing argument types. > - despite what's advertised, passing a GLib.Bytes in place of a GByteArray or > guint8* doesn't work, one needs an explicit toArray(). While requiring it for > mutability may be ok, I don't like it much just for passing bytes around. The way I've always thought of introspection is to enable "scripting C" type code. So in this example, in C, you'd need to make an explicit copy too. What APIs were you trying to use that took a guint8* or a GByteArray? I and others have been adding GBytes variants to parts of the stack (GLib, libsoup), since it's nicer even for C, especially when you have asynchronous methods. For example, g_output_stream_write_async() predates GBytes; g_output_stream_write_bytes_async() is just a lot nicer. > - despite what's advertised, passing a ByteArray in place of a guint8*+length > works but takes a copy. To avoid that, you need to take the fastpath in > to_explicit_array_internal() instead, as arrays with length sidestep > gjs_value_to_g_argument(). Ah, good point. Will fix. But to repeat the above, I'd like to also fix this by adding GBytes API where possible to native code. > - passing an Array in place of GBytes doesn't work, one needs an explicit > GLib.Bytes.new, which involves a copy. This in particular affects GVariant's > autopacking, as it cannot just shove the value down to > g_variant_new_from_bytes(). By "Array" here you mean "JS array"? We can't avoid a copy in that case, at least, not right now - to my knowedge anyways. It would clearly be nice to be able to take a typed JS array e.g. and have GBytes reference the underlying storage...but we likely have bigger performance issues to solve. > - GLib.Bytes.new() and new GLib.Bytes() don't recognize if the passed in value > is a ByteArray, so they may incur an unneeded copy. Right...hrm. To fix we'd need to have a custom override for .new() - will do.
(In reply to comment #16) > > > - despite what's advertised, passing a ByteArray in place of a guint8*+length > > works but takes a copy. To avoid that, you need to take the fastpath in > > to_explicit_array_internal() instead, as arrays with length sidestep > > gjs_value_to_g_argument(). > > Ah, good point. Will fix. But to repeat the above, I'd like to also fix this > by adding GBytes API where possible to native code. So...this is a bit uglier than I thought, because we need to recongize the "is GBytes" case when we're doing the initial conversion, and then also remember later that we don't need to call g_free() on the result. I think I'd like to rewrite the whole function invocation path so that we push all allocations we do internally during argument conversion onto a "cleanup list" (GList of struct { gpointer data; GDestroyNotify destroy)). Then we just walk the cleanup list after the call, rather than walking the argument list again and inspecting all of the argument types *again*.
Created attachment 228843 [details] [review] byteArray: Convert static initialization to g_once(), factor out Using g_once() helps us find static initialization places, and is threadsafe in case that ever matters. Also, factoring out into a separate function will help a future patch.
Created attachment 228844 [details] [review] Add new GBytes API and conversions GLib's annotations for GBytes have been updated so one can now do: new GLib.Bytes([1,2,3]) This allows us to construct them natively, which is really nice. But we should go further. Now, we should have the ability to convert them to a JS array. In gjs, we have the byteArray class which "looks like" a JS array, but actually points to a native array, since it's a lot more efficient. So, change the internal byteArray class so that it supports holding a GBytes *or* a GByteArray. It's used as a container as well as a bridge from JS -> native conversions for the overrides code. Add an override "toArray" that allows converting a GBytes to a "gjs byteArray", which in turn has automatic conversions to e.g. guint8+len pair as used from C.
Review of attachment 228843 [details] [review]: OK. Btw, that function exists to ensure that the module and class objects exists, and in turn it ensures that JS_InitClass has been called, causing gjs_byte_array_prototype to be valid for the later call to JS_NewObject.
Comment on attachment 228843 [details] [review] byteArray: Convert static initialization to g_once(), factor out I updated the comment with your text, thanks =) Attachment 228843 [details] pushed as 58cddea - byteArray: Convert static initialization to g_once(), factor out
Any further thoughts on this GBytes patch, Giovanni? The last patch isn't too much changed from earlier revisions, but I will do the argument processing cleanup at some point this cycle, which will allow us to add byteArray -> guint8*+len conversion.
(In reply to comment #22) > Any further thoughts on this GBytes patch, Giovanni? The last patch isn't too > much changed from earlier revisions, but I will do the argument processing > cleanup at some point this cycle, which will allow us to add byteArray -> > guint8*+len conversion. In that case it would be ok with me. I've been testing it recently, and saw no major regression, including after converting GVariant marshalling to go through GBytes, so I'd say go. There's plenty of time to fix any problem before 3.8.
Review of attachment 228844 [details] [review]: But, before you go, please change the commit message to reflect what's actually possible with the new API.
Attachment 228844 [details] pushed as 94cb4e9 - Add new GBytes API and conversions