After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 604076 - C invocation speedups
C invocation speedups
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2009-12-08 14:07 UTC by Colin Walters
Modified: 2010-02-17 22:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Optimize function invocation (23.78 KB, patch)
2009-12-08 14:07 UTC, Colin Walters
reviewed Details | Review
Cache interface information (3.53 KB, patch)
2009-12-08 14:07 UTC, Colin Walters
needs-work Details | Review
Stack allocate arg and type infos for invocation (7.57 KB, patch)
2009-12-08 14:07 UTC, Colin Walters
reviewed Details | Review
Optimize function invocation (24.88 KB, patch)
2010-01-06 21:04 UTC, Colin Walters
reviewed Details | Review
Stack allocate arg and type infos for invocation (13.50 KB, patch)
2010-01-06 21:04 UTC, Colin Walters
none Details | Review
[function.c] Fix up invocation processing failure (9.56 KB, patch)
2010-02-12 20:45 UTC, Colin Walters
needs-work Details | Review
Add VALGRIND_ARGS variable to Makefile (829 bytes, patch)
2010-02-17 19:42 UTC, Colin Walters
committed Details | Review
[function.c] Fix up invocation processing failure (14.48 KB, patch)
2010-02-17 19:42 UTC, Colin Walters
none Details | Review
Optimize function invocation, fix allocation cleanup (35.34 KB, patch)
2010-02-17 20:01 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2009-12-08 14:07:29 UTC
See patches, depends on https://bugzilla.gnome.org/show_bug.cgi?id=604074
Comment 1 Colin Walters 2009-12-08 14:07:33 UTC
Created attachment 149331 [details] [review]
Optimize function invocation

Rather than using the "toy" API g_function_info_invoke, looking up
the function info each time, cache the prepared libffi cif inside
Function.  This applies to method invocations; constructors still
fall back to a slow path.
Comment 2 Colin Walters 2009-12-08 14:07:44 UTC
Created attachment 149332 [details] [review]
Cache interface information

Looking up the interfaces for an object is a rather expensive
operation right now in introspection; instead of doing it
each time we're doing an interface invocation, cache them in
the object.
Comment 3 Colin Walters 2009-12-08 14:07:54 UTC
Created attachment 149333 [details] [review]
Stack allocate arg and type infos for invocation

Introspection now supports a stack-allocated API for getting
argument information; use it and avoid malloc'ing repeatedly
in the invocation fast path.
Comment 4 Johan (not receiving bugmail) Dahlin 2009-12-08 18:07:51 UTC
Review of attachment 149331 [details] [review]:

Nice, this is looking pretty good, a couple of quick comments:

::: gi/function.c
@@ +46,3 @@
+    guint js_out_argc;
+    guint expected_js_argc;
+    guint n_destroy_notifies;

guint8 for all these? + putting them at the end of the struct

@@ +468,3 @@
     int argv_pos;
     int i;
+    int in_args_pos, out_args_pos;

I think all these can be int8 too, no?

@@ +894,3 @@
 };
+function_init (JSContext      *context,
+static gboolean

init is a fairly bad name, but I don't have any considerable better suggestions, prep_invoke?

@@ +898,3 @@
+               Function       *function,
+function_init (JSContext      *context,
+static gboolean

guint8 here too

@@ +1042,3 @@
+JSBool
+
+

Function function = { 0, }; instead of calling memset directly?
What about letting the init function do the allocation?
Comment 5 Johan (not receiving bugmail) Dahlin 2009-12-08 18:14:31 UTC
Review of attachment 149332 [details] [review]:

Looks okay, but what about invalidation? Is there a reason to invalidate this cache at any time?
What's the cache hit % and how many interfaces are normally stored (taking up how many bytes)

::: gi/object.c
@@ +44,3 @@
     GObject *gobj; /* NULL if we are the prototype and not an instance */
     GIObjectInfo *info;
+    guint n_interfaces;

guint8 I think

@@ +657,3 @@
         GType gtype;
     if (!is_proto) {
+        guint n_interfaces, i;

Ditto.
Also n_interfaces doesn't need to be a local variable afact

@@ +782,3 @@
     ObjectInstance *priv;
 {
+    guint i;

And ditto
Comment 6 Havoc Pennington 2009-12-08 18:21:21 UTC
Comment on attachment 149332 [details] [review]
Cache interface information

this is kind of gross from a memory-usage perspective, no, to make this per instance? shouldn't it be per class somehow? maybe GIObjectInfo in g-i should cache it, or maybe we should have a "class" blob for the JS wrapper, or I'm not sure. anyway bloating up ObjectInstance is undesirable if we can help it
Comment 7 Johan (not receiving bugmail) Dahlin 2009-12-08 18:23:40 UTC
Review of attachment 149333 [details] [review]:

Great! I now get why you used memset in the previous patch, ignore that comment.
Comment 8 Colin Walters 2009-12-17 02:55:34 UTC
(In reply to comment #6)
> (From update of attachment 149332 [details] [review])
> this is kind of gross from a memory-usage perspective, no, to make this per
> instance? shouldn't it be per class somehow?

Hmm, yes.  I'll look at this.

> maybe GIObjectInfo in g-i should
> cache it,

Possibly.  There's clearly some things we could do here in introspection (I had a work-in-progress patch for using an index in the typelib).  But that aside, adding another caching layer in introspection is tricky, especially if we're moving to allowing stack allocation of the info structs.
Comment 9 Colin Walters 2009-12-17 02:59:40 UTC
(In reply to comment #5)
> Review of attachment 149332 [details] [review]:
> 
> Looks okay, but what about invalidation? Is there a reason to invalidate this
> cache at any time?

No reason to invalidate, the interfaces from the typelib can't change.

> What's the cache hit % and how many interfaces are normally stored (taking up
> how many bytes)

Well, how many interfaces are used depends on which classes your app is accessing.  I'm pretty sure the memory used by caching the info structs for interfaces of used classes is going to be pretty tiny though.  I'll see if I can optimize this though to actually be per-class and get some numbers.
Comment 10 Colin Walters 2010-01-06 20:29:56 UTC
(In reply to comment #6)
> (From update of attachment 149332 [details] [review])
> this is kind of gross from a memory-usage perspective, no, to make this per
> instance? shouldn't it be per class somehow? maybe GIObjectInfo in g-i should
> cache it, or maybe we should have a "class" blob for the JS wrapper, or I'm not
> sure. anyway bloating up ObjectInstance is undesirable if we can help it

I think what we need to do here is make a PrototypeObjectInstance which holds this data on the prototype, and have ObjectInstance just hold the GObject pointer.  

This is a nontrivial patch though, and I don't want to block the function invocation speedups from landing before doing this, so I'm moving this to a separate bug.
Comment 11 Colin Walters 2010-01-06 21:04:03 UTC
Created attachment 150931 [details] [review]
Optimize function invocation

Rather than using the "toy" API g_function_info_invoke, looking up
the function info each time, cache the prepared libffi cif inside
Function.  This applies to method invocations; constructors still
fall back to a slow path.
Comment 12 Colin Walters 2010-01-06 21:04:09 UTC
Created attachment 150932 [details] [review]
Stack allocate arg and type infos for invocation

Introspection now supports a stack-allocated API for getting
argument information; use it and avoid malloc'ing repeatedly
in the invocation fast path.
Comment 13 Colin Walters 2010-01-06 21:04:59 UTC
Patches updated to master, and should address comment #4
Comment 14 Colin Walters 2010-01-06 21:08:26 UTC
By the way, see https://bugzilla.gnome.org/show_bug.cgi?id=604074 for my benchmark and timings.  Executive summary: about 33% faster, which in the realm of benchmarks is obviously enormous.
Comment 15 Owen Taylor 2010-02-11 23:24:50 UTC
Review of attachment 150931 [details] [review]:

After 20-30 minutes looking this, the only real concern I have is about the memory freeing paths on invocation - you've changed the logic there in ways that don't make sense to me. I could be completely offbase and what you have is fine, but I think it's worth another look.

::: gi/boxed.c
@@ +238,3 @@
                 rval = JSVAL_NULL;
+                gjs_invoke_c_function_uncached(context, func_info, obj,
+                                               0, NULL, &rval);

Should be at least a bug filed to extend the infrastructure to handle this as well, perhaps? - some boxed constructors might be called a whole lot.

::: gi/function.c
@@ +622,3 @@
     /* gjs_value_to_g_arg should have reported a JS error if we failed, return FALSE */
+    if (failed) {
+        /* FIXME release any partially converted arguments */

The logic is hard to decipher here, certainly, but I think you've regressed this and it was fine before.

@@ +701,3 @@
         arg_type_info = g_arg_info_get_type(arg_info);
 
+        if (direction == GI_DIRECTION_IN || direction == GI_DIRECTION_INOUT) {

It really looks wrong to me to call gjs_g_argument_release_in_arg() on an inout argument. Is that intentional? Is it related to the rest of this patch?

@@ +717,3 @@
+            arg = &out_arg_cvalues[out_args_pos];
+
+            if (!failed &&

This change - checking 'failed' here, to me is not right. The function, whether or not, local_error is set, whether or not any previous arguments failed conversion,  should just continue and convert all the arguments. Once everything is convert, you can let the GC clean up, as the code did before.

@@ +769,3 @@
+    if (failed && local_error == NULL) {
+        return JS_FALSE;
+    } else if (!failed) {

failed, not failed, failed, is pretty ugly.

 if (failed) {
    if (local_error != NULL) {
    }

    return JS_FALSE;
 }

 return JS_TRUE;

@@ +904,3 @@
+    GITypeInfo *return_type;
+
+    if (!g_function_info_prep_invoker (info, &(function->invoker), &error)) {

space before (, and throughout this function
Comment 16 Colin Walters 2010-02-12 20:45:02 UTC
Created attachment 153662 [details] [review]
[function.c] Fix up invocation processing failure

The way processing failure was handled was pretty broken; we had
just a single global "failed" argument, which we set to TRUE if
at any point something failed.

But this would cause memory leaks if for example we failed in the
middle of releasing input arguments; we'd never free the remaining
args.

Clean this up by introducing different variables which we use at
different points.
Comment 17 Colin Walters 2010-02-12 20:49:21 UTC
(In reply to comment #15)
> 
> The logic is hard to decipher here, certainly, but I think you've regressed
> this and it was fine before.

It was broken before in some cases, but yes.

> @@ +701,3 @@
>          arg_type_info = g_arg_info_get_type(arg_info);
> 
> +        if (direction == GI_DIRECTION_IN || direction == GI_DIRECTION_INOUT) {
> 
> It really looks wrong to me to call gjs_g_argument_release_in_arg() on an inout
> argument. Is that intentional? Is it related to the rest of this patch?

Yes, we converted the inout arg potentially; think of (inout) char * where we converted to UTF-8 to pass in, while the caller reset the output pointer we still have a pointer to the original converted argument which we need to free.

> @@ +904,3 @@
> +    GITypeInfo *return_type;
> +
> +    if (!g_function_info_prep_invoker (info, &(function->invoker), &error)) {
> 
> space before (, and throughout this function

Haven't fixed this, actually gjs style is no space.  I've been accidentally introducing them in some places and will remove them before committing.
Comment 18 Colin Walters 2010-02-12 20:49:55 UTC
Comment on attachment 149332 [details] [review]
Cache interface information

marking as needs-work based on comment 6
Comment 19 Owen Taylor 2010-02-12 22:56:30 UTC
Review of attachment 153662 [details] [review]:

::: gi/function.c
@@ +462,3 @@
     GArgument *in_arg_cvalues;
     GArgument *out_arg_cvalues;
     gpointer *in_arg_pointers;

I'd really like to see a comment explaining the relationship between these in the different directions in a nice table.

@@ +672,1 @@
                 failed = TRUE;

I don't think setting failed here is appropriate - that will mean that we won't try to release any other out arguments? but the function we called has no idea that gjs_value_from_g_argument_failed() so it probably stored allocated memory there.

I think I might be most comfortable without a global failed - and just have the various individual types of failure and combine them into the return value.

@@ +702,3 @@
         g_arg_info_load_type(&arg_info, &arg_type_info);
 
+        if ((direction == GI_DIRECTION_IN || direction == GI_DIRECTION_INOUT) &&

The INOUT case here is still, I think, not right.

 in_arg_cvalues[in_args_pos] is going to be &out_arg_cvalues[out_arg_pos]

which has been overwritten with the return value. Plus, gjs_g_argument_release_in_arg() doesn't look at the direction, so it can't know that it has an extra dereference to make. Presumably you'd want to release the original out_arg_cvalues[], which is stored away somewhere. 

Also, this badly needs a comment that in the INOUT case we are freeing the original value, not the returned value, since it's pretty weird otherwise to first free and then convert.

Actually, in the transfer_none case I think it's probably legitimate for the caller to leave the inout parameter untouched, so you probably should convert than free anyways.
Comment 20 Colin Walters 2010-02-17 19:42:23 UTC
Created attachment 154069 [details] [review]
Add VALGRIND_ARGS variable to Makefile
Comment 21 Colin Walters 2010-02-17 19:42:33 UTC
Created attachment 154070 [details] [review]
[function.c] Fix up invocation processing failure

The way processing failure was handled was pretty broken; we had
just a single global "failed" argument, which we set to TRUE if
at any point something failed.

But this would cause memory leaks if for example we failed in the
middle of releasing input arguments; we'd never free the remaining
args.

Clean this up by introducing different variables which we use at
different points.
Comment 22 Colin Walters 2010-02-17 19:48:04 UTC
This could use some more tests for sure (we need one that has multiple inout at different positions etc.), but I'm decently confident in it and I'm sure it's less buggy than what came before.

valgrind-check shows no regressions[1], and GNOME Shell runs OK. 

I'd like to commit this now that 0.5 is out, and let it bake in git for a bit.  Any objections?

[1] Though we need to add the current stuff to suppressions so we can automatically say "make valgrind-check must pass"
Comment 23 Colin Walters 2010-02-17 19:49:02 UTC
Also any preferences for squashing the 3 commits together (not quite bisectable) or committed as one?  I'm actually leaning towards the latter.
Comment 24 Colin Walters 2010-02-17 20:01:54 UTC
Created attachment 154076 [details] [review]
Optimize function invocation, fix allocation cleanup

Rather than using the "toy" API g_function_info_invoke, looking up
the function info each time, cache the prepared libffi cif inside
Function.  This applies to method invocations; constructors still
fall back to a slow path.

Introspection now supports a stack-allocated API for getting
argument information; use it and avoid malloc'ing repeatedly
in the invocation fast path.

The way processing failure was handled was pretty broken; we had
just a single global "failed" argument, which we set to TRUE if
at any point something failed.

But this would cause memory leaks if for example we failed in the
middle of releasing input arguments; we'd never free the remaining
args.

Clean this up by introducing different variables which we use at
different points.
Comment 25 Johan (not receiving bugmail) Dahlin 2010-02-17 20:06:07 UTC
No objections, this should go in ASAP. Squashing the commits together sounds fine.

Would be nice to preserve the API though, is there a reason to remove the old and add new the new _uncached function, from an API point of view?
Comment 26 Colin Walters 2010-02-17 22:17:26 UTC
Attachment 154069 [details] pushed as 5c8e08b - Add VALGRIND_ARGS variable to Makefile
Attachment 154076 [details] pushed as 47381c9 - Optimize function invocation, fix allocation cleanup
Comment 27 Colin Walters 2010-02-17 22:26:37 UTC
(In reply to comment #25)
> No objections, this should go in ASAP. Squashing the commits together sounds
> fine.
> 
> Would be nice to preserve the API though, is there a reason to remove the old
> and add new the new _uncached function, from an API point of view?

Hm well I wanted to mark that it was an expensive function.  I don't think it's really what I'd call "public" API exactly.  The stuff from "context.h" is really the only things most consumers of gjs should ever have to use.