GNOME Bugzilla – Bug 615222
Massively clean up callback invocation
Last modified: 2010-04-15 14:32:32 UTC
For callbacks, it greatly simplifies the code to make one fundamental assumption: There is at most one (callback,user_data,GDestroyNotify) triple per function. Multiple (scope call) callbacks are OK, but supporting say two callbacks with GDestroyNotify is really painful implementation-wise, and I'm not aware of a function with such a signature at the moment that lives in the introspectable stack. (GHashTable has one, but it doesn't count being in GLib). Once we have this first assumption, note that we can have a single GDestroyNotify callback instance, and pass in for the "user_data" what we need into that destroy notify. This is another huge simplification since there's no longer two different kinds of callbacks (one for actual callbacks and one for GDestroyNotify). Also, I made things more efficient; e.g. there's no longer a pointless g_slice for the ffi_cif; we can just pack the cif into the larger structure.
Created attachment 158236 [details] [review] Massively clean up callback invocation
Created attachment 158238 [details] [review] [function.c] Rename variables for clarity (no functional changes) Because it's easy to be confused about whether variables named like "argc" are referring to the JS or C side, attempt to more consistently prefix the JS ones with "js_". This looks more natural than prefixing the C ones.
The second patch is logically before the first. Also I forgot to note that I made passing "undefined" for a callback an error; I can't think of how that would be useful.
Review of attachment 158238 [details] [review]: This looks good
Review of attachment 158236 [details] [review]: ::: gi/function.c @@ +43,2 @@ typedef struct { Everything related to callbacks probably belongs in gi/callbacks.c would be nice to have that done too, but it should be done in a later patch. @@ +69,3 @@ + * will be freed the next time we invoke a C function. + * while it's in use, this list keeps track of ones that +/* Because we can't free the mmap'd data for a callback We use camel case and the Gjs suffix for the other structs. Should probably include "DestroyNotify" as a part of it's name @@ +77,3 @@ + ffi_closure *closure; + ffi_cif cif; + jsval js_function; Saving the GIScopeType instead of that guint would make a bit more sense here, even thought it would use a few more bits. @@ +124,3 @@ { + g_callable_info_free_closure(trampoline->info, trampoline->closure); + JS_RemoveRoot(trampoline->context, &trampoline->js_function); Is this unreffing the trampoline->info? If not -> leaking. @@ +198,2 @@ + if (trampoline->free_after_invocation) { +out: .. this code here would be clearer with the change above. @@ +198,3 @@ + completed_trampolines = g_slist_prepend (completed_trampolines, trampoline); + if (trampoline->free_after_invocation) { +out: Extra space before ( @@ +211,3 @@ { void *data) + GjsCallbackTrampoline *trampoline = *(void**)(args[0]); Can we assume that args always contain at least one element? @@ +219,3 @@ +gjs_init_callback_statics () +static void +/* Called when we first see a function that uses a callback */ gjs_create_static_destroy_notify_trampoline or so is a better name (void) right? Can/Should we reuse the closure allocation logic from g-i here? Seems a bit of duplication going on. @@ +235,3 @@ if (status != FFI_OK) { (ffi_type**)destroy_notify_args); + g_error ("ffi_prep_cif failed: %d\n", status); Extra space before * @@ +243,2 @@ if (status != FFI_OK) { + g_error ("ffi_prep_cif failed: %d\n", status); Ditto space @@ +243,3 @@ if (status != FFI_OK) { + return; + g_error ("ffi_prep_cif failed: %d\n", status); ->cif is leaked, not sure if we care since it's a global. @@ +265,1 @@ + trampoline = g_new0(GjsCallbackTrampoline, 1); All variables will be initializes, so g_new instead of g_new0 or even better, use g_slice_new. @@ +342,3 @@ can_throw_gerror = (flags & GI_FUNCTION_THROWS) != 0; is_method = (flags & GI_FUNCTION_IS_METHOD) != 0; + n_args = g_callable_info_get_n_args( (GICallableInfo*) function->info); Extra space, I'd do s/( (/((/g @@ +358,2 @@ } + /* Check if we have a callback; if so, process all the arguments (callback, destroy_notify, user_data) Consider moving this to a separate function @@ +360,3 @@ + */ + * at once to avoid having special cases in the main loop below. + /* Check if we have a callback; if so, process all the arguments (callback, destroy_notify, user_data) #define INDEX_UNSET G_MAXUINT8 ? @@ +368,3 @@ + */ + * at once to avoid having special cases in the main loop below. + /* Check if we have a callback; if so, process all the arguments (callback, destroy_notify, user_data) Ditto, the same in quite a few other places, consistency is good for ehum, consistency :) @@ +377,3 @@ + /* Find the JS function passed for the callback */ + found_js_function = FALSE; + js_function = JSVAL_NULL; Why both of these? @@ +396,3 @@ + g_base_info_get_name( (GIBaseInfo*) function->info), + g_base_info_get_name( (GIBaseInfo*) &callback_arg)); + return FALSE; calback_info is leaked @@ +399,3 @@ + } + + callback_trampoline = gjs_callback_trampoline_new(context, js_function, Add a comment explaining where and why this is freed here, I thought it was a leak first. @@ +873,3 @@ + if (type_tag == GI_TYPE_TAG_INTERFACE) { + type_tag = g_type_info_get_tag(&type_info); + g_arg_info_load_type(&arg_info, &type_info); I wish we had more macros/convenience functions for this in g-i itself. @@ +877,3 @@ + if (type_tag == GI_TYPE_TAG_INTERFACE) { + type_tag = g_type_info_get_tag(&type_info); + g_arg_info_load_type(&arg_info, &type_info); Include namespace in the error message. @@ +930,3 @@ + && function->destroy_notify_index != G_MAXUINT8 + if (function->callback_index != G_MAXUINT8 + function->info is leaked ::: test/js/testEverythingBasic.js @@ +204,3 @@ assertEquals('CallbackNull', Everything.test_callback(null), 0); + assertRaises('CallbackUndefined', function () { Everything.test_callback(undefined) }); This makes sense, good catch.
Comment on attachment 158238 [details] [review] [function.c] Rename variables for clarity (no functional changes) Attachment 158238 [details] pushed as 6c82b1d - [function.c] Rename variables for clarity (no functional changes)
(In reply to comment #5) > > @@ +69,3 @@ > + * will be freed the next time we invoke a C function. > + * while it's in use, this list keeps track of ones that > +/* Because we can't free the mmap'd data for a callback > > We use camel case and the Gjs suffix for the other structs. Should probably > include "DestroyNotify" as a part of it's name You're talking about the "global_destroy_notify" struct? In that case it's not the name of the struct, but the actual variable. The struct's anonymous. > @@ +124,3 @@ > { > + g_callable_info_free_closure(trampoline->info, trampoline->closure); > + JS_RemoveRoot(trampoline->context, &trampoline->js_function); > > Is this unreffing the trampoline->info? If not -> leaking. Fixed. > Extra space before ( Fixed. > > @@ +211,3 @@ > { > void *data) > + GjsCallbackTrampoline *trampoline = *(void**)(args[0]); > > Can we assume that args always contain at least one element? Yeah, that's the specification for calling a GDestroyNotify. > @@ +219,3 @@ > +gjs_init_callback_statics () > +static void > +/* Called when we first see a function that uses a callback */ > > gjs_create_static_destroy_notify_trampoline or so is a better name Well I sort of assumed that there might be a possibility for adding more statics later. Ok to keep this? > > (void) right? > > Can/Should we reuse the closure allocation logic from g-i here? Seems a bit of > duplication going on. I changed it to look up GDestroyNotify from the repository. > @@ +265,1 @@ > + trampoline = g_new0(GjsCallbackTrampoline, 1); > > All variables will be initializes, so g_new instead of g_new0 or even better, > use g_slice_new. Ok. > > @@ +342,3 @@ > can_throw_gerror = (flags & GI_FUNCTION_THROWS) != 0; > is_method = (flags & GI_FUNCTION_IS_METHOD) != 0; > + n_args = g_callable_info_get_n_args( (GICallableInfo*) function->info); > > Extra space, I'd do s/( (/((/g That extra space for the cast is all over gjs code... > > @@ +358,2 @@ > } > + /* Check if we have a callback; if so, process all the arguments > (callback, destroy_notify, user_data) > > Consider moving this to a separate function Done. > #define INDEX_UNSET G_MAXUINT8 ? Done. > @@ +377,3 @@ > + /* Find the JS function passed for the callback */ > + found_js_function = FALSE; > + js_function = JSVAL_NULL; > > Why both of these? Well we need to allow passing JSVAL_NULL for js_function. > > @@ +396,3 @@ > + g_base_info_get_name( (GIBaseInfo*) function->info), > + g_base_info_get_name( (GIBaseInfo*) &callback_arg)); > + return FALSE; > > calback_info is leaked Fixed. > @@ +877,3 @@ > + if (type_tag == GI_TYPE_TAG_INTERFACE) { > + type_tag = g_type_info_get_tag(&type_info); > + g_arg_info_load_type(&arg_info, &type_info); > > Include namespace in the error message. Fixed. > @@ +930,3 @@ > + && function->destroy_notify_index != G_MAXUINT8 > + if (function->callback_index != G_MAXUINT8 > + > > function->info is leaked Hmm this should be freed in function_finalize but the _uncached path may not do so...I'll investigate. This wouldn't be a bug introduced in this patch though.
Created attachment 158303 [details] [review] Massively clean up callback invocation For callbacks, it greatly simplifies the code to make one fundamental assumption: There is at most one (callback,user_data,GDestroyNotify) triple per function. Multiple (scope call) callbacks are OK, but supporting say two callbacks with GDestroyNotify is really painful implementation-wise, and I'm not aware of a function with such a signature at the moment that lives in the introspectable stack. (GHashTable has one, but it doesn't count being in GLib). Once we have this first assumption, note that we can have a single GDestroyNotify callback instance, and pass in for the "user_data" what we need into that destroy notify. This is another huge simplification since there's no longer two different kinds of callbacks (one for actual callbacks and one for GDestroyNotify). Also, I made things more efficient; e.g. there's no longer a pointless g_slice for the ffi_cif; we can just pack the cif into the larger structure. Finally, this patch makes passing "undefined" for a callback an explicit error. I can't think how that's useful. You can still pass "null" of course.
Created attachment 158757 [details] [review] [function.c] Free cached invoker data, also free in _uncached The _uncached invocation path wasn't freeing the cached function data; make it do so. Also, for both the regular and _uncached path we need to free the invoker data.
Comment on attachment 158757 [details] [review] [function.c] Free cached invoker data, also free in _uncached Looks good!
Attachment 158303 [details] pushed as 26f89d6 - Massively clean up callback invocation Attachment 158757 [details] pushed as bed86b4 - [function.c] Free cached invoker data, also free in _uncached