[gjs/wip/gcampax/70-arg-cache: 5/5] function: use the argument cache for the instance parameter too

[Philip Chimento <pchimento src gnome org>]

commit ecd0bdb3b372197471a2e1ec4a483c35b48aa3d2
Author: Giovanni Campagna <gcampagna src gnome org>
Date:   Mon Apr 22 19:21:10 2013 +0200

    function: use the argument cache for the instance parameter too
    
    This frees us of expensive allocations and switches in another
    hot path.
    
    Closes #70.
    
    (Philip Chimento: Rebased and fixed coding style and bugs.)

 gi/arg-cache.cpp | 106 +++++++++++++++++++++++++++++++++--
 gi/arg-cache.h   |   2 +
 gi/function.cpp  | 164 +++++++++++++++----------------------------------------
 3 files changed, 146 insertions(+), 126 deletions(-)
---
diff --git a/gi/arg-cache.cpp b/gi/arg-cache.cpp
index dde44ce2..3f3e0dbe 100644
--- a/gi/arg-cache.cpp
+++ b/gi/arg-cache.cpp
@@ -44,6 +44,7 @@
 #include "gi/gerror.h"
 #include "gi/gtype.h"
 #include "gi/object.h"
+#include "gi/param.h"
 #include "gi/union.h"
 #include "gi/value.h"
 #include "gjs/byteArray.h"
@@ -685,6 +686,58 @@ static bool gjs_marshal_object_in_in(JSContext* cx, GjsArgumentCache* self,
                                                self->transfer, gtype);
 }
 
+GJS_JSAPI_RETURN_CONVENTION
+static bool gjs_marshal_gtype_struct_instance_in(JSContext* cx,
+                                                 GjsArgumentCache* self,
+                                                 GjsFunctionCallState*,
+                                                 GIArgument* arg,
+                                                 JS::HandleValue value) {
+    // Instance parameter is never nullable
+    if (!value.isObject())
+        return report_primitive_type_mismatch(cx, self, value, JSTYPE_OBJECT);
+
+    JS::RootedObject obj(cx, &value.toObject());
+    GType actual_gtype;
+    if (!gjs_gtype_get_actual_gtype(cx, obj, &actual_gtype))
+        return false;
+
+    if (actual_gtype == G_TYPE_NONE) {
+        gjs_throw(cx, "Invalid GType class passed for instance parameter");
+        return false;
+    }
+
+    // We use peek here to simplify reference counting (we just ignore transfer
+    // annotation, as GType classes are never really freed.) We know that the
+    // GType class is referenced at least once when the JS constructor is
+    // initialized.
+    if (g_type_is_a(actual_gtype, G_TYPE_INTERFACE))
+        arg->v_pointer = g_type_default_interface_peek(actual_gtype);
+    else
+        arg->v_pointer = g_type_class_peek(actual_gtype);
+
+    return true;
+}
+
+GJS_JSAPI_RETURN_CONVENTION
+static bool gjs_marshal_param_instance_in(JSContext* cx, GjsArgumentCache* self,
+                                          GjsFunctionCallState*,
+                                          GIArgument* arg,
+                                          JS::HandleValue value) {
+    // Instance parameter is never nullable
+    if (!value.isObject())
+        return report_primitive_type_mismatch(cx, self, value, JSTYPE_OBJECT);
+
+    JS::RootedObject obj(cx, &value.toObject());
+    if (!gjs_typecheck_param(cx, obj, G_TYPE_PARAM, true))
+        return false;
+    arg->v_pointer = gjs_g_param_from_param(cx, obj);
+
+    if (self->transfer == GI_TRANSFER_EVERYTHING)
+        g_param_spec_ref(static_cast<GParamSpec*>(arg->v_pointer));
+
+    return true;
+}
+
 GJS_JSAPI_RETURN_CONVENTION
 static bool gjs_marshal_skipped_out(JSContext*, GjsArgumentCache*,
                                     GjsFunctionCallState*, GIArgument*,
@@ -983,9 +1036,8 @@ static inline bool is_gdk_atom(GIBaseInfo* info) {
            strcmp("Gdk", g_base_info_get_namespace(info)) == 0;
 }
 
-static bool gjs_arg_cache_build_interface_in_arg(GjsArgumentCache* self) {
-    GjsAutoBaseInfo interface_info =
-        g_type_info_get_interface(&self->type_info);
+static bool gjs_arg_cache_build_interface_in_arg(GjsArgumentCache* self,
+                                                 GIBaseInfo* interface_info) {
     GIInfoType interface_type = g_base_info_get_type(interface_info);
 
     // We do some transfer magic later, so let's ensure we don't mess up.
@@ -1179,8 +1231,11 @@ static bool gjs_arg_cache_build_normal_in_arg(GjsArgumentCache* self,
             self->contents.string_is_filename = false;
             break;
 
-        case GI_TYPE_TAG_INTERFACE:
-            return gjs_arg_cache_build_interface_in_arg(self);
+        case GI_TYPE_TAG_INTERFACE: {
+            GjsAutoBaseInfo interface_info =
+                g_type_info_get_interface(&self->type_info);
+            return gjs_arg_cache_build_interface_in_arg(self, interface_info);
+        }
 
         case GI_TYPE_TAG_ARRAY:
         case GI_TYPE_TAG_GLIST:
@@ -1196,6 +1251,47 @@ static bool gjs_arg_cache_build_normal_in_arg(GjsArgumentCache* self,
     return true;
 }
 
+bool gjs_arg_cache_build_instance(GjsArgumentCache* self,
+                                  GICallableInfo* info) {
+    GIBaseInfo* interface_info = g_base_info_get_container(info);  // unowned
+
+    self->arg_pos = -2;
+    self->arg_name = "instance parameter";
+    self->transfer = g_callable_info_get_instance_ownership_transfer(info);
+    // Some calls accept null for the instance, but generally in an object
+    // oriented language it's wrong to call a method on null
+    self->nullable = false;
+    self->skip_out = true;
+
+    // These cases could be covered by the generic marshaller, except that
+    // there's no way to get GITypeInfo for a method's instance parameter.
+    // Instead, special-case the arguments here that would otherwise go through
+    // the generic marshaller.
+    // See: https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/334
+    GIInfoType info_type = g_base_info_get_type(interface_info);
+    if (info_type == GI_INFO_TYPE_STRUCT &&
+        g_struct_info_is_gtype_struct(interface_info)) {
+        self->marshal_in = gjs_marshal_gtype_struct_instance_in;
+        self->release = gjs_marshal_skipped_release;
+        return true;
+    }
+    if (info_type == GI_INFO_TYPE_OBJECT) {
+        GType gtype = g_registered_type_info_get_g_type(interface_info);
+
+        if (g_type_is_a(gtype, G_TYPE_PARAM)) {
+            self->marshal_in = gjs_marshal_param_instance_in;
+            self->release = gjs_marshal_skipped_release;
+            return true;
+        }
+    }
+
+    bool ok = gjs_arg_cache_build_interface_in_arg(self, interface_info);
+    // Don't set marshal_out, it should not be called for the instance
+    // parameter
+    self->release = gjs_marshal_skipped_release;
+    return ok;
+}
+
 bool gjs_arg_cache_build_arg(GjsArgumentCache* self,
                              GjsArgumentCache* arguments, int gi_index,
                              GIDirection direction, GIArgInfo* arg_info,
diff --git a/gi/arg-cache.h b/gi/arg-cache.h
index 2944c75b..809f3c6c 100644
--- a/gi/arg-cache.h
+++ b/gi/arg-cache.h
@@ -110,6 +110,8 @@ bool gjs_arg_cache_build_return(GjsArgumentCache* self,
                                 GjsArgumentCache* arguments,
                                 GICallableInfo* info, bool* inc_counter_out);
 
+bool gjs_arg_cache_build_instance(GjsArgumentCache* self, GICallableInfo* info);
+
 GJS_USE
 static inline bool gjs_arg_cache_is_skip_in(GjsArgumentCache* cache) {
     return cache->skip_in;
diff --git a/gi/function.cpp b/gi/function.cpp
index 8c78a9cb..730fe5f9 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -613,110 +613,6 @@ GjsCallbackTrampoline* gjs_callback_trampoline_new(
     return trampoline;
 }
 
-GJS_JSAPI_RETURN_CONVENTION
-static bool
-gjs_fill_method_instance(JSContext       *context,
-                         JS::HandleObject obj,
-                         Function        *function,
-                         GIArgument      *out_arg,
-                         bool&            is_gobject)
-{
-    GIBaseInfo *container = g_base_info_get_container((GIBaseInfo *) function->info);
-    GIInfoType type = g_base_info_get_type(container);
-    GType gtype = g_registered_type_info_get_g_type ((GIRegisteredTypeInfo *)container);
-    GITransfer transfer = g_callable_info_get_instance_ownership_transfer (function->info);
-
-    is_gobject = false;
-
-    if (type == GI_INFO_TYPE_STRUCT || type == GI_INFO_TYPE_BOXED) {
-        /* GError must be special cased */
-        if (g_type_is_a(gtype, G_TYPE_ERROR)) {
-            if (!ErrorBase::transfer_to_gi_argument(context, obj, out_arg,
-                                                    GI_DIRECTION_OUT, transfer))
-                return false;
-        } else if (type == GI_INFO_TYPE_STRUCT &&
-                   g_struct_info_is_gtype_struct((GIStructInfo*) container)) {
-            /* And so do GType structures */
-            GType actual_gtype;
-            gpointer klass;
-
-            if (!gjs_gtype_get_actual_gtype(context, obj, &actual_gtype))
-                return false;
-
-            if (actual_gtype == G_TYPE_NONE) {
-                gjs_throw(context, "Invalid GType class passed for instance parameter");
-                return false;
-            }
-
-            /* We use peek here to simplify reference counting (we just ignore
-               transfer annotation, as GType classes are never really freed)
-               We know that the GType class is referenced at least once when
-               the JS constructor is initialized.
-            */
-
-            if (g_type_is_a(actual_gtype, G_TYPE_INTERFACE))
-                klass = g_type_default_interface_peek(actual_gtype);
-            else
-                klass = g_type_class_peek(actual_gtype);
-
-            out_arg->v_pointer = klass;
-        } else {
-            if (!BoxedBase::transfer_to_gi_argument(context, obj, out_arg,
-                                                    GI_DIRECTION_OUT, transfer,
-                                                    gtype, container))
-                return false;
-        }
-
-    } else if (type == GI_INFO_TYPE_UNION) {
-        if (!UnionBase::transfer_to_gi_argument(context, obj, out_arg,
-                                                GI_DIRECTION_OUT, transfer,
-                                                gtype, container))
-            return false;
-
-    } else if (type == GI_INFO_TYPE_OBJECT || type == GI_INFO_TYPE_INTERFACE) {
-        if (g_type_is_a(gtype, G_TYPE_OBJECT)) {
-            if (!ObjectBase::transfer_to_gi_argument(
-                    context, obj, out_arg, GI_DIRECTION_OUT, transfer, gtype))
-                return false;
-            is_gobject = true;
-        } else if (g_type_is_a(gtype, G_TYPE_PARAM)) {
-            if (!gjs_typecheck_param(context, obj, G_TYPE_PARAM, true))
-                return false;
-            out_arg->v_pointer = gjs_g_param_from_param(context, obj);
-            if (transfer == GI_TRANSFER_EVERYTHING)
-                g_param_spec_ref ((GParamSpec*) out_arg->v_pointer);
-        } else if (G_TYPE_IS_INTERFACE(gtype)) {
-            if (ObjectBase::check_jsclass(context, obj)) {
-                if (!ObjectBase::transfer_to_gi_argument(context, obj, out_arg,
-                                                         GI_DIRECTION_OUT,
-                                                         transfer, gtype))
-                    return false;
-                is_gobject = true;
-            } else {
-                if (!FundamentalBase::transfer_to_gi_argument(
-                        context, obj, out_arg, GI_DIRECTION_OUT, transfer,
-                        gtype))
-                    return false;
-            }
-        } else if (G_TYPE_IS_INSTANTIATABLE(gtype)) {
-            if (!FundamentalBase::transfer_to_gi_argument(
-                    context, obj, out_arg, GI_DIRECTION_OUT, transfer, gtype))
-                return false;
-        } else {
-            gjs_throw_custom(context, JSProto_TypeError, nullptr,
-                             "%s.%s is not an object instance neither a fundamental instance of a supported 
type",
-                             g_base_info_get_namespace(container),
-                             g_base_info_get_name(container));
-            return false;
-        }
-
-    } else {
-        g_assert_not_reached();
-    }
-
-    return true;
-}
-
 /* Intended for error messages. Return value must be freed */
 GJS_USE
 static char *
@@ -774,7 +670,6 @@ gjs_invoke_c_function(JSContext                             *context,
     bool failed, postinvoke_release_failed;
 
     bool is_method;
-    bool is_object_method = false;
     GITypeTag return_tag;
     JS::RootedValueVector return_values(context);
 
@@ -853,10 +748,14 @@ gjs_invoke_c_function(JSContext                             *context,
     js_arg_pos = 0; /* index into argv */
 
     if (is_method) {
-        if (!gjs_fill_method_instance(context, obj, function,
-                                      &state.in_cvalues[-2], is_object_method))
+        GjsArgumentCache* cache = &function->arguments[-2];
+        GIArgument* in_value = &state.in_cvalues[-2];
+        JS::RootedValue in_js_value(context, JS::ObjectValue(*obj));
+
+        if (!cache->marshal_in(context, cache, &state, in_value, in_js_value))
             return false;
-        ffi_arg_pointers[0] = &state.in_cvalues[-2];
+
+        ffi_arg_pointers[ffi_arg_pos] = in_value;
         ++ffi_arg_pos;
     }
 
@@ -983,12 +882,14 @@ gjs_invoke_c_function(JSContext                             *context,
 release:
     // In this loop we use ffi_arg_pos just to ensure we don't release stuff
     // we haven't allocated yet, if we failed in type conversion above.
-    // Because we start from -1 (the return value), we need to process 1 more
-    // than processed_c_args
+    // If we start from -1 (the return value), we need to process 1 more than
+    // processed_c_args.
+    // If we start from -2 (the instance parameter), we need to process 2 more
     ffi_arg_pos = is_method ? 1 : 0;
     postinvoke_release_failed = false;
-    for (gi_arg_pos = -1;
-         gi_arg_pos < gi_argc && ffi_arg_pos < (processed_c_args + 1);
+    for (gi_arg_pos = is_method ? -2 : -1;
+         gi_arg_pos < gi_argc &&
+         ffi_arg_pos < (processed_c_args + (is_method ? 2 : 1));
          gi_arg_pos++, ffi_arg_pos++) {
         GjsArgumentCache* cache = &function->arguments[gi_arg_pos];
         GIArgument* in_value = &state.in_cvalues[gi_arg_pos];
@@ -1013,7 +914,7 @@ release:
     if (postinvoke_release_failed)
         failed = true;
 
-    g_assert(ffi_arg_pos == processed_c_args + 1);
+    g_assert(ffi_arg_pos == processed_c_args + (is_method ? 2 : 1));
 
     if (function->js_out_argc > 0 && (!failed && !did_throw_gerror)) {
         /* if we have 1 return value or out arg, return that item
@@ -1082,12 +983,18 @@ GJS_NATIVE_CONSTRUCTOR_DEFINE_ABSTRACT(function)
 static void
 uninit_cached_function_data (Function *function)
 {
-    g_clear_pointer(&function->info, g_base_info_unref);
+    // Careful! function->arguments is one/two inside an array
+    if (function->arguments) {
+        bool is_method = g_callable_info_is_method(function->info);
+
+        if (is_method)
+            g_free(&function->arguments[-2]);
+        else
+            g_free(&function->arguments[-1]);
+        function->arguments = nullptr;
+    }
 
-    // Careful! function->arguments is one inside an array
-    if (function->arguments)
-        g_free(&function->arguments[-1]);
-    function->arguments = nullptr;
+    g_clear_pointer(&function->info, g_base_info_unref);
 
     g_function_invoker_destroy(&function->invoker);
 }
@@ -1262,11 +1169,26 @@ init_cached_function_data (JSContext      *context,
         }
     }
 
+    bool is_method = g_callable_info_is_method(info);
     n_args = g_callable_info_get_n_args((GICallableInfo*) info);
 
-    // arguments is one inside an array of n_args + 1, so arguments[-1] is the
-    // return value (if any)
-    GjsArgumentCache* arguments = g_new0(GjsArgumentCache, n_args + 1) + 1;
+    // arguments is one or two inside an array of n_args + 2, so
+    // arguments[-1] is the return value (which can be skipped if void)
+    // arguments[-2] is the instance parameter
+    size_t offset = is_method ? 2 : 1;
+    GjsArgumentCache* arguments =
+        g_new0(GjsArgumentCache, n_args + offset) + offset;
+
+    if (is_method) {
+        if (!gjs_arg_cache_build_instance(&arguments[-2], info)) {
+            gjs_throw(context,
+                      "Function %s.%s cannot be called: the instance parameter "
+                      "is not introspectable.",
+                      g_base_info_get_namespace((GIBaseInfo*)info),
+                      g_base_info_get_name((GIBaseInfo*)info));
+            return false;
+        }
+    }
 
     bool inc_counter;
     if (!gjs_arg_cache_build_return(&arguments[-1], arguments, info,