Created attachment 149326 [details] [review]
[girffi] g_function_info_prep_invoker

Rather than having bindings use g_function_info_invoke, which is basically
a toy/demo API, export a convenience utility function which takes the introspection
information and sets up things we need to pass to libffi.

Then invocation can be done directly to libffi by a binding.

Created attachment 149327 [details] [review]
Allow stack allocating GIBaseInfo

We don't want to malloc each GIBaseInfo when they can be used in
function invocation; instead, allow stack allocation.

There were a lot of structure typedefs which were actually just
exactly the same as GIBaseInfo, with the one exception of GITypeInfo.

Instead, just put the single GITypeInfo boolean inside GIBaseInfo
as a bit in a bitfield.

GIBaseInfo is still opaque publicly; GIBaseInfoReal is the new
internal structure.

Add two new functions to retrieve arguments and argument types
on the stack.

Review of attachment 149326 [details] [review]:

::: girepository/girffi.c
@@ +287,3 @@
 }
 
   return g_ir_ffi_get_ffi_type (type_tag);

Would be nice to have a couple of tests for this.

@@ +289,3 @@
 }
+g_function_info_prep_invoker (GIFunctionInfo       *info,
+gboolean

Missing documentation

@@ +301,3 @@
+                              GIFunctionInvoker    *invoker,
+g_function_info_prep_invoker (GIFunctionInfo       *info,
+gboolean

Make sure the input arguments aren't NULL

@@ +334,3 @@
+
+  /* TODO: avoid malloc here? */
+  atypes = g_malloc0 (sizeof (ffi_type*) * n_invoke_args);

Yes, why not use g_alloca()?

@@ +369,3 @@
+}
+
+void

Missing documentation here too.

@@ +370,3 @@
+
+void
+g_function_info_invoker_destroy (GIFunctionInvoker    *invoker)

g_function_invoker_destroy()

::: girepository/girffi.h
@@ +34,3 @@
+struct _GIFunctionInvoker {
+
+typedef struct _GIFunctionInvoker GIFunctionInvoker;

Is it necessary to expose this structure, can't we hide it?
I guess the native_address is main interesting part, we could use a setter for that.

(In reply to comment #3)
> 
> +  /* TODO: avoid malloc here? */
> +  atypes = g_malloc0 (sizeof (ffi_type*) * n_invoke_args);
> 
> Yes, why not use g_alloca()?

Because this function returns a structure which is expected to have a lifecycle longer than the stack; e.g. it's intended to be cached in gjs' Function object or equivalent.

> 
> Is it necessary to expose this structure, can't we hide it?
> I guess the native_address is main interesting part, we could use a setter for
> that.

Callers need access to the libffi cif at least; that's the intent for the gjs patch which then can more directly marshal its arguments into libffi.

Created attachment 149484 [details] [review]
[girffi] Clean up API, add g_function_info_prep_invoker

Rather than having bindings use g_function_info_invoke, which is basically
a toy/demo API, export a convenience utility function which takes the introspection
information and sets up things we need to pass to libffi.

Then invocation can be done directly to libffi by a binding.

As part of this work, remove some (unused by gjs) public functions from the
girffi API, and instead export a function to map to libffi which can work
semi-correctly.

Review of attachment 149327 [details] [review]:

Patch generally make sense to me; as mentioned in person, I think this bug report should have information about:

 - What measurements you did before writing this patch that indicated that this was a bottleneck
 - What the observed change with this patch

Otherwise it will be hard for someone looking at this change retrospectively to get a sense of why it was made.

(I do wonder why the existing code wasn't using GSlice; not that it's much faster than a good malloc(), but it's what it was designed for.)

::: girepository/ginfo.c
@@ +31,3 @@
+typedef struct _GIBaseInfoReal GIBaseInfoReal;
+
+struct _GIBaseInfoReal

I'm not sure GIBaseInfoReal is the best name for something which with this change *has* to have the fields for every subtype; since I just read through thousands of lines that change 'base' to 'rinfo', maybe just calls this GIRealInfo ?

@@ +62,3 @@
 
+static void
+g_info_init (GIBaseInfoReal *info,

I think it would be a good idea to flag stack-allocated GIBaseInfo as stack allocated and detect and assert if g_base_info_unref() drops the refcount on such a object to zero. Somebody might think they could malloc space for an ArgInfo, then let it be freed, but that would screw up refcounts, and cause really hard to find problems.

@@ +82,1 @@
+  g_assert (G_IREPOSITORY (repository));

why isn't this G_IS_IREPOSITORY? (I don't see that macro defined, but that's strange too.)

@@ -149,4 +89,4 @@
-		 GIRepository  *repository,
-		 GIBaseInfo    *container,
-		 GTypelib     *typelib, 
-		 guint32        offset)
+                 GIRepository  *repository,
+		         GIBaseInfo    *container,
+                 GTypelib     *typelib, 
+                 guint32        offset)

Pointless reindentation ending up with bad indentation

@@ -179,3 +112,3 @@
-	    GIBaseInfo    *container,
-	    GTypelib     *typelib, 
-	    guint32        offset)
+    	    GIBaseInfo    *container,
+	        GTypelib     *typelib, 
+	        guint32        offset)

Indentation problems

@@ -187,3 +120,3 @@
 g_info_from_entry (GIRepository *repository,
-		   GTypelib *typelib,
-		   guint16    index)
+		           GTypelib     *typelib,
+		           guint16       index)

Indentation problems

@@ -202,3 +135,3 @@
       if (result == NULL)
-	{
-	  GIUnresolvedInfo *unresolved;
+        {
+          GIUnresolvedInfo *unresolved;

This whole section of code got reindented spaces to tabs or something

@@ +148,2 @@
+          return (GIBaseInfo*)unresolved;
+	    }

This isn't just reindented, it's reindented wrong

@@ +365,3 @@
 const gchar *
 g_base_info_get_attribute (GIBaseInfo   *info,
+            			   const gchar *name)

Indentation problems

@@ +605,3 @@
 g_type_info_new (GIBaseInfo    *container,
+        		 GTypelib     *typelib,
+		         guint32        offset)

Indentation problems

@@ +611,3 @@
+  return (GITypeInfo *) g_info_new (GI_INFO_TYPE_TYPE, container, typelib, 
+		                      		     (type->flags.reserved == 0 && type->flags.reserved2 == 0)
+		                      		        ? offset : type->offset);

Indentation problems

@@ +733,3 @@
+
+/**
+ * g_callable_info_iter_arg:

The docs here should document what the function does, not just the difference in memory management (could by reference to g_callable_info_get_arg())

@@ +736,3 @@
+ * @info: a #GICallableInfo
+ * @n: the argument index to fetch
+ * @arg: Initialize arg, lifecycle is chained to @info

This isn't very decipherable, maybe something like:

 @arg: a GIArgInfo structure that has been allocated by the caller.
       It will be initialized with information about the specified argument.
       The information stored can be as long as @info exists.

@@ +859,3 @@
+
+/**
+ * g_arg_info_iter_type:

Again, you don't document what the function does

@@ +861,3 @@
+ * g_arg_info_iter_type:
+ * @info: A #GIArgInfo
+ * @type: A #GITypeInfo, initialized

"A #GITypeInfo, initialized"?

@@ +865,3 @@
+ * Rather than allocating a new #GITypeInfo, initialize an existing structure
+ * allocated by the caller.  The initialized @type is valid as long as @info
+ * is.  Do not call g_base_info_unref() on it.

You don't actually say what the function does here! (Could be by reference to g_arg_info_get_type())

@@ +947,3 @@
+  if (rinfo->type_is_embedded)
+    return (GIBaseInfo *) g_info_new (type->offset, (GIBaseInfo*)info, rinfo->typelib,
+		                 		      rinfo->offset);

Identation

@@ +1048,3 @@
+        return (GIErrorDomainInfo *) g_info_from_entry (rinfo->repository,
+	                                					rinfo->typelib,
+		                            					blob->domains[n]);

Indentation problems

@@ +1232,2 @@
+  return (GIFieldInfo *) g_info_new (GI_INFO_TYPE_FIELD, (GIBaseInfo*)info, rinfo->typelib, 
+				                     g_struct_get_field_offset (info, n));

Indentation problems

@@ +1276,3 @@
       if (strcmp (name, fname) == 0)
+        return (GIFunctionInfo *) g_info_new (GI_INFO_TYPE_FUNCTION, base, 
+	                        			      rinfo->typelib, offset);  

Identation problems

@@ +1602,3 @@
             const gchar  *name)
 {
+  GIBaseInfoReal *rinfo = (GIBaseInfoReal*)base;

Would be a little more compact if you just made find_vfunc() take GIBaseInfoReal

::: girepository/girepository.h
@@ +38,3 @@
+
+struct _GIBaseInfoStub {
+  gpointer padding[12];

I think it would be better to do:

 gint dummy1;
 gint dummy2;
 gpointer dummy3;
 gpointer dummy4;
 gpointer dummy5;
 guint32  dummy6;
 guint    dummy7 : 1;
 gpointer padding[4];

Or something so that you are documenting how much padding there is. (and also, so you pad consistently across architectures instead of having an extra pointer of padding on 64-bit.)

@@ +297,3 @@
+void                   g_callable_info_iter_arg         (GICallableInfo *info,
+							                             gint           n,
+							                             GIArgInfo     *arg);

Indentation problems.

Name seems a little hard to understand to me - I'd expect g_callable_info_iter_arg() to be related to iterating over the arguments in some fashion and be puzzled by g_arg_info_iter_type.

Maybe g_callable_iter_get_arg_local() ?

@@ +329,3 @@
 GITypeInfo *           g_arg_info_get_type               (GIArgInfo *info);
+void                   g_arg_info_iter_type              (GIArgInfo *info,
+                                                          GITypeInfo *type);

It seems strange to me to do this for just a couple of getters, while leaving other getters that are pretty similar in performance needs unchanged.

E.g., every time gjs reads a field from a structure it uses g_struct_info_get_field() and g_field_info_get_type().

(In reply to comment #6)
> 
> Pointless reindentation ending up with bad indentation

This turned out to be lots of lurking tabs in introspection code =(  I removed the tabs as I went along; what you pointed out should be fixed.

> Name seems a little hard to understand to me - I'd expect
> g_callable_info_iter_arg() to be related to iterating over the arguments in
> some fashion and be puzzled by g_arg_info_iter_type.
> 
> Maybe g_callable_iter_get_arg_local() ?

I decided on g_callable_info_load_arg().

> 
> @@ +329,3 @@
>  GITypeInfo *           g_arg_info_get_type               (GIArgInfo *info);
> +void                   g_arg_info_iter_type              (GIArgInfo *info,
> +                                                          GITypeInfo *type);
> 
> It seems strange to me to do this for just a couple of getters, while leaving
> other getters that are pretty similar in performance needs unchanged.
> 
> E.g., every time gjs reads a field from a structure it uses
> g_struct_info_get_field() and g_field_info_get_type().

Yeah but it wasn't showing up in profiles.  We have other ones to fix, some harder than others, such as g_type_info_get_interface which is complicated in how it calls back into the repository to look up other typelibs.

I'll fix as we go, and for integration in GLib a definite goal will be to remove all the mallocing accessors.

Created attachment 149882 [details]
loop js benchmark

Benchmark I used for tests.

Created attachment 149883 [details]
wall clock timing data 

Just old + new timings; I don't have broken out per-patch changes.  But IIRC, malloc overhead was about 2 full seconds of the loop benchmark.  We were mallocing a *lot*.

Attachment 149484 [details] pushed as 8df0648 - [girffi] Clean up API, add g_function_info_prep_invoker

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]