[gjs/wip/gcampax/70-arg-cache: 1/6] FIXME function: introduce a new system for better caching of marshalling info



commit e2b52f5e75c5cc5e5b1ca652a08f430457158c39
Author: Giovanni Campagna <gcampagna src gnome org>
Date:   Fri Apr 19 20:27:07 2013 +0200

    FIXME function: introduce a new system for better caching of marshalling info
    
    The marshalling metadata, as stored in the typelib, is not a format
    friendly to gjs, and requires to have multiple switches and a complex
    logic in the hot path of function calls.
    Instead, extend the current caching mechanism to allow arbitrary
    data to be attached to arguments, and to have it deal with JS arguments,
    rather than a mixture of C and GI arguments.
    
    FIXME: Contains a use-after-free somewhere.
    
    (Philip Chimento: rebased and fixed coding style.)

 gi/arg-cache.cpp | 190 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gi/arg-cache.h   |  74 ++++++++++++++++++++++
 gi/function.cpp  | 185 +++++++++++++++++++++++++++++------------------------
 gjs-srcs.mk      |   2 +
 4 files changed, 369 insertions(+), 82 deletions(-)
---
diff --git a/gi/arg-cache.cpp b/gi/arg-cache.cpp
new file mode 100644
index 00000000..6b01af92
--- /dev/null
+++ b/gi/arg-cache.cpp
@@ -0,0 +1,190 @@
+/* -*- mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (c) 2013 Giovanni Campagna <scampa giovanni gmail com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <config.h>
+
+#include <string.h>
+
+#include <girepository.h>
+#include <glib.h>
+
+#include "arg.h"
+#include "arg-cache.h"
+#include "function.h"
+
+bool
+gjs_arg_cache_build_return(GjsArgumentCache *self,
+                           GjsParamType     *param_types,
+                           GICallableInfo   *info,
+                           bool&             inc_counter)
+{
+    GITypeInfo return_type;
+    g_callable_info_load_return_type(info, &return_type);
+
+    inc_counter = g_type_info_get_tag(&return_type) != GI_TYPE_TAG_VOID;
+
+    int length_pos = g_type_info_get_array_length(&return_type);
+    if (length_pos >= 0)
+        param_types[length_pos] = PARAM_SKIPPED;
+
+    if (inc_counter)
+        self->param_type = PARAM_NORMAL;
+    else
+        self->param_type = PARAM_SKIPPED;
+
+    return true;
+}
+
+bool
+gjs_arg_cache_build_in_arg(GjsArgumentCache *self,
+                           GjsParamType     *param_types,
+                           int               gi_index,
+                           GIArgInfo        *arg_info,
+                           bool&             inc_counter)
+{
+    GITypeInfo type_info;
+    g_arg_info_load_type(arg_info, &type_info);
+
+    self->param_type = PARAM_NORMAL;
+    inc_counter = true;
+
+    GITypeTag type_tag = g_type_info_get_tag(&type_info);
+    if (type_tag == GI_TYPE_TAG_INTERFACE) {
+        GIBaseInfo *interface_info = g_type_info_get_interface(&type_info);
+        GIInfoType interface_type = g_base_info_get_type(interface_info);
+        if (interface_type == GI_INFO_TYPE_CALLBACK) {
+            if (strcmp(g_base_info_get_name(interface_info), "DestroyNotify") == 0 &&
+                strcmp(g_base_info_get_namespace(interface_info), "GLib") == 0) {
+                /* Skip GDestroyNotify if they appear before the respective
+                 * callback */
+                self->param_type = PARAM_SKIPPED;
+                inc_counter = false;
+            } else {
+                self->param_type = PARAM_CALLBACK;
+
+                int destroy_pos = g_arg_info_get_destroy(arg_info);
+                int closure_pos = g_arg_info_get_closure(arg_info);
+
+                if (destroy_pos >= 0)
+                    param_types[destroy_pos] = PARAM_SKIPPED;
+
+                if (closure_pos >= 0)
+                    param_types[closure_pos] = PARAM_SKIPPED;
+
+                if (destroy_pos >= 0 && closure_pos < 0) {
+                    /* Function has a GDestroyNotify but no user_data, not
+                     * supported */
+                    g_base_info_unref(interface_info);
+                    return false;
+                }
+            }
+
+            g_base_info_unref(interface_info);
+        }
+    } else if (type_tag == GI_TYPE_TAG_ARRAY) {
+        if (g_type_info_get_array_type(&type_info) == GI_ARRAY_TYPE_C) {
+            int length_pos = g_type_info_get_array_length(&type_info);
+
+            if (length_pos >= 0) {
+                param_types[length_pos] = PARAM_SKIPPED;
+                self->param_type = PARAM_ARRAY;
+
+                if (length_pos < gi_index) {
+                    /* we already collected length_pos, remove it */
+                    inc_counter = false;
+                }
+            }
+        }
+    }
+
+    return true;
+}
+
+bool
+gjs_arg_cache_build_out_arg(GjsArgumentCache *self,
+                            GjsParamType     *param_types,
+                            int               gi_index,
+                            GIArgInfo        *arg_info,
+                            bool&             inc_counter)
+{
+    GITypeInfo type_info;
+    g_arg_info_load_type(arg_info, &type_info);
+
+    self->param_type = PARAM_NORMAL;
+    inc_counter = true;
+
+    GITypeTag type_tag = g_type_info_get_tag(&type_info);
+    if (type_tag == GI_TYPE_TAG_ARRAY) {
+        if (g_type_info_get_array_type(&type_info) == GI_ARRAY_TYPE_C) {
+            int length_pos = g_type_info_get_array_length(&type_info);
+
+            if (length_pos >= 0) {
+                param_types[length_pos] = PARAM_SKIPPED;
+                self->param_type = PARAM_ARRAY;
+
+                if (length_pos < gi_index) {
+                    /* we already collected length_pos, remove it */
+                    inc_counter = false;
+                }
+            }
+        }
+    }
+
+    return true;
+}
+
+bool
+gjs_arg_cache_build_inout_arg(GjsArgumentCache *in_self,
+                              GjsArgumentCache *out_self,
+                              GjsParamType     *param_types,
+                              int               gi_index,
+                              GIArgInfo        *arg_info,
+                              bool&             inc_counter)
+{
+    GITypeInfo type_info;
+    g_arg_info_load_type(arg_info, &type_info);
+
+    in_self->param_type = PARAM_NORMAL;
+    out_self->param_type = PARAM_NORMAL;
+    inc_counter = true;
+
+    GITypeTag type_tag = g_type_info_get_tag(&type_info);
+    if (type_tag == GI_TYPE_TAG_ARRAY) {
+        if (g_type_info_get_array_type(&type_info) == GI_ARRAY_TYPE_C) {
+            int length_pos = g_type_info_get_array_length(&type_info);
+
+            if (length_pos >= 0) {
+                param_types[length_pos] = PARAM_SKIPPED;
+                in_self->param_type = PARAM_ARRAY;
+                out_self->param_type = PARAM_ARRAY;
+
+                if (length_pos < gi_index) {
+                    /* we already collected length_pos, remove it */
+                    inc_counter = false;
+                }
+            }
+        }
+    }
+
+    return true;
+}
diff --git a/gi/arg-cache.h b/gi/arg-cache.h
new file mode 100644
index 00000000..0ed8e185
--- /dev/null
+++ b/gi/arg-cache.h
@@ -0,0 +1,74 @@
+/* -*- mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (c) 2013 Giovanni Campagna <scampa giovanni gmail com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef GJS_ARG_CACHE_H
+#define GJS_ARG_CACHE_H
+
+#include <glib.h>
+#include <girepository.h>
+
+#include "function.h"
+#include "gjs/jsapi-wrapper.h"
+
+G_BEGIN_DECLS
+
+typedef struct _GjsArgumentCache {
+    bool (*marshal) (struct _GjsArgumentCache *, GIArgument *, JS::HandleValue);
+    bool (*release) (struct _GjsArgumentCache *, GIArgument *);
+    bool (*free) (struct _GjsArgumentCache *);
+
+    /* For compatibility */
+    GjsParamType param_type;
+
+    union {
+        int dummy;
+    } contents;
+} GjsArgumentCache;
+
+bool gjs_arg_cache_build_in_arg(GjsArgumentCache *self,
+                                GjsParamType     *param_types,
+                                int               gi_index,
+                                GIArgInfo        *arg,
+                                bool&             inc_counter);
+
+bool gjs_arg_cache_build_out_arg(GjsArgumentCache *self,
+                                 GjsParamType     *param_types,
+                                 int               gi_index,
+                                 GIArgInfo        *arg,
+                                 bool&             inc_counter);
+
+bool gjs_arg_cache_build_inout_arg(GjsArgumentCache *in_self,
+                                   GjsArgumentCache *out_self,
+                                   GjsParamType     *param_types,
+                                   int               gi_index,
+                                   GIArgInfo        *arg,
+                                   bool&             inc_counter);
+
+bool gjs_arg_cache_build_return(GjsArgumentCache *self,
+                                GjsParamType     *param_types,
+                                GICallableInfo   *info,
+                                bool&             inc_counter);
+
+G_END_DECLS
+
+#endif  /* GJS_ARG_CACHE_H */
diff --git a/gi/function.cpp b/gi/function.cpp
index 97573da0..4453701c 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -23,8 +23,9 @@
 
 #include <config.h>
 
-#include "function.h"
 #include "arg.h"
+#include "arg-cache.h"
+#include "function.h"
 #include "object.h"
 #include "fundamental.h"
 #include "boxed.h"
@@ -54,8 +55,10 @@ typedef struct {
     GIFunctionInfo *info;
 
     GjsParamType *param_types;
+    GjsArgumentCache *in_arguments;
+    GjsArgumentCache *out_arguments;
 
-    guint8 expected_js_argc;
+    uint8_t js_in_argc;
     guint8 js_out_argc;
     GIFunctionInvoker invoker;
 } Function;
@@ -800,22 +803,22 @@ gjs_invoke_c_function(JSContext                             *context,
     /* @c_argc is the number of arguments that the underlying C
      * function takes. @gi_argc is the number of arguments the
      * GICallableInfo describes (which does not include "this" or
-     * GError**). @function->expected_js_argc is the number of
+     * GError**). @function->js_in_argc is the number of
      * arguments we expect the JS function to take (which does not
      * include PARAM_SKIPPED args).
      *
      * @js_argc is the number of arguments that were actually passed.
      */
-    if (args.length() > function->expected_js_argc) {
+    if (args.length() > function->js_in_argc) {
         GjsAutoChar name = format_function_name(function, is_method);
         JS_ReportWarningUTF8(context, "Too many arguments to %s: expected %d, "
                              "got %" G_GSIZE_FORMAT, name.get(),
-                             function->expected_js_argc, args.length());
-    } else if (args.length() < function->expected_js_argc) {
+                             function->js_in_argc, args.length());
+    } else if (args.length() < function->js_in_argc) {
         GjsAutoChar name = format_function_name(function, is_method);
         gjs_throw(context, "Too few arguments to %s: "
                   "expected %d, got %" G_GSIZE_FORMAT,
-                  name.get(), function->expected_js_argc, args.length());
+                  name.get(), function->js_in_argc, args.length());
         return false;
     }
 
@@ -1556,6 +1559,18 @@ static JSFunctionSpec gjs_function_proto_funcs[] = {
 
 static JSFunctionSpec *gjs_function_static_funcs = nullptr;
 
+static void
+throw_not_introspectable_argument(JSContext      *cx,
+                                  GICallableInfo *function,
+                                  GIArgInfo      *arg)
+{
+    gjs_throw(cx, "Function %s.%s cannot be called: argument '%s' is not "
+              "introspectable.",
+              g_base_info_get_namespace(function),
+              g_base_info_get_name(function),
+              g_base_info_get_name(arg));
+}
+
 static bool
 init_cached_function_data (JSContext      *context,
                            Function       *function,
@@ -1563,9 +1578,7 @@ init_cached_function_data (JSContext      *context,
                            GICallableInfo *info)
 {
     guint8 i, n_args;
-    int array_length_pos;
     GError *error = NULL;
-    GITypeInfo return_type;
     GIInfoType info_type;
 
     info_type = g_base_info_get_type((GIBaseInfo *)info);
@@ -1597,103 +1610,111 @@ init_cached_function_data (JSContext      *context,
         }
     }
 
-    g_callable_info_load_return_type((GICallableInfo*)info, &return_type);
-    if (g_type_info_get_tag(&return_type) != GI_TYPE_TAG_VOID)
-        function->js_out_argc += 1;
-
     n_args = g_callable_info_get_n_args((GICallableInfo*) info);
+
+    /* We preallocate structures conservatively, then we copy in dynamic memory
+     * only those we need.
+     * Ideally, there would be a 1:1 mapping between GjsArgumentCache and JS
+     * arguments, but we need to handle the case of a length argument happening
+     * before its corresponding array, so we allow for "holes"
+     * (GjsArgumentCache whose param_type is PARAM_SKIPPED), and in_args maps
+     * to GI arguments, while out_args maps to GI arguments and the return
+     * value. To ease handling, out_args is actually one inside the array (so
+     * out_args[-1] is the return value).
+     * In any case, there is a 1:1 mapping between GI arguments and
+     * function->param_types. */
+    GjsArgumentCache *in_args = g_newa(GjsArgumentCache, n_args);
+    GjsArgumentCache *out_args = g_newa(GjsArgumentCache, n_args + 1) + 1;
+    /*memset(in_args, sizeof(GjsArgumentCache) * n_args, 0);
+      memset(out_args - 1, sizeof(GjsArgumentCache) * (n_args + 1), 0);*/
     function->param_types = g_new0(GjsParamType, n_args);
 
-    array_length_pos = g_type_info_get_array_length(&return_type);
-    if (array_length_pos >= 0 && array_length_pos < n_args)
-        function->param_types[array_length_pos] = PARAM_SKIPPED;
+    bool inc_counter;
+    if (!gjs_arg_cache_build_return(&out_args[-1], function->param_types,
+                                    info, inc_counter)) {
+        gjs_throw(context, "Function %s.%s cannot be called: the return value "
+                  "is not introspectable.",
+                  g_base_info_get_namespace(info),
+                  g_base_info_get_name(info));
+        return false;
+    }
+    int out_argc = inc_counter ? 1 : 0;
+    int in_argc = 0;
 
     for (i = 0; i < n_args; i++) {
         GIDirection direction;
         GIArgInfo arg_info;
-        GITypeInfo type_info;
-        int destroy = -1;
-        int closure = -1;
-        GITypeTag type_tag;
 
         if (function->param_types[i] == PARAM_SKIPPED)
             continue;
 
         g_callable_info_load_arg((GICallableInfo*) info, i, &arg_info);
-        g_arg_info_load_type(&arg_info, &type_info);
-
         direction = g_arg_info_get_direction(&arg_info);
-        type_tag = g_type_info_get_tag(&type_info);
 
-        if (type_tag == GI_TYPE_TAG_INTERFACE) {
-            GIBaseInfo* interface_info;
-            GIInfoType interface_type;
+        if (direction == GI_DIRECTION_IN) {
+            if (!gjs_arg_cache_build_in_arg(&in_args[i], function->param_types,
+                                            i, &arg_info, inc_counter)) {
+                throw_not_introspectable_argument(context, info, &arg_info);
+                return false;
+            }
 
-            interface_info = g_type_info_get_interface(&type_info);
-            interface_type = g_base_info_get_type(interface_info);
-            if (interface_type == GI_INFO_TYPE_CALLBACK) {
-                if (strcmp(g_base_info_get_name(interface_info), "DestroyNotify") == 0 &&
-                    strcmp(g_base_info_get_namespace(interface_info), "GLib") == 0) {
-                    /* Skip GDestroyNotify if they appear before the respective callback */
-                    function->param_types[i] = PARAM_SKIPPED;
-                } else {
-                    function->param_types[i] = PARAM_CALLBACK;
-                    function->expected_js_argc += 1;
+            function->param_types[i] = in_args[i].param_type;
+            if (inc_counter)
+                in_argc++;
+        } else if (direction == GI_DIRECTION_INOUT) {
+            if (!gjs_arg_cache_build_inout_arg(&in_args[i], &out_args[i],
+                                               function->param_types,
+                                               i, &arg_info, inc_counter)) {
+                throw_not_introspectable_argument(context, info, &arg_info);
+                return false;
+            }
 
-                    destroy = g_arg_info_get_destroy(&arg_info);
-                    closure = g_arg_info_get_closure(&arg_info);
+            function->param_types[i] = in_args[i].param_type;
+            if (inc_counter) {
+                in_argc++;
+                out_argc++;
+            }
+        } else { /* GI_DIRECTION_OUT */
+            if (out_args[i].param_type == PARAM_SKIPPED)
+                continue;
 
-                    if (destroy >= 0 && destroy < n_args)
-                        function->param_types[destroy] = PARAM_SKIPPED;
+            if (!gjs_arg_cache_build_out_arg(&out_args[i],
+                                             function->param_types,
+                                             i, &arg_info, inc_counter)) {
+                throw_not_introspectable_argument(context, info, &arg_info);
+                return false;
+            }
 
-                    if (closure >= 0 && closure < n_args)
-                        function->param_types[closure] = PARAM_SKIPPED;
+            function->param_types[i] = out_args[i].param_type;
+            if (inc_counter)
+                out_argc++;
+        }
+    }
 
-                    if (destroy >= 0 && closure < 0) {
-                        gjs_throw(context, "Function %s.%s has a GDestroyNotify but no user_data, not 
supported",
-                                  g_base_info_get_namespace( (GIBaseInfo*) info),
-                                  g_base_info_get_name( (GIBaseInfo*) info));
-                        g_base_info_unref(interface_info);
-                        return false;
-                    }
-                }
-            }
-            g_base_info_unref(interface_info);
-        } else if (type_tag == GI_TYPE_TAG_ARRAY) {
-            if (g_type_info_get_array_type(&type_info) == GI_ARRAY_TYPE_C) {
-                array_length_pos = g_type_info_get_array_length(&type_info);
+    function->in_arguments = g_new(GjsArgumentCache, in_argc);
+    function->out_arguments = g_new(GjsArgumentCache, out_argc);
 
-                if (array_length_pos >= 0 && array_length_pos < n_args) {
-                    GIArgInfo length_arg_info;
+    function->js_in_argc = function->js_out_argc = 0;
+    if (out_args[-1].param_type != PARAM_SKIPPED) {
+        function->out_arguments[0] = out_args[-1];
+        function->js_out_argc = 1;
+    }
 
-                    g_callable_info_load_arg((GICallableInfo*) info, array_length_pos, &length_arg_info);
-                    if (g_arg_info_get_direction(&length_arg_info) != direction) {
-                        gjs_throw(context, "Function %s.%s has an array with different-direction length arg, 
not supported",
-                                  g_base_info_get_namespace( (GIBaseInfo*) info),
-                                  g_base_info_get_name( (GIBaseInfo*) info));
-                        return false;
-                    }
+    for (i = 0; i < n_args; i++) {
+        if (function->param_types[i] == PARAM_SKIPPED)
+            continue;
 
-                    function->param_types[array_length_pos] = PARAM_SKIPPED;
-                    function->param_types[i] = PARAM_ARRAY;
+        GIArgInfo arg_info;
+        g_callable_info_load_arg((GICallableInfo*) info, i, &arg_info);
+        GIDirection direction = g_arg_info_get_direction(&arg_info);
 
-                    if (array_length_pos < i) {
-                        /* we already collected array_length_pos, remove it */
-                        if (direction == GI_DIRECTION_IN || direction == GI_DIRECTION_INOUT)
-                            function->expected_js_argc -= 1;
-                        if (direction == GI_DIRECTION_OUT || direction == GI_DIRECTION_INOUT)
-                            function->js_out_argc -= 1;
-                    }
-                }
-            }
+        if (direction == GI_DIRECTION_IN || direction == GI_DIRECTION_INOUT) {
+            function->in_arguments[function->js_in_argc] = in_args[i];
+            function->js_in_argc++;
         }
-
-        if (function->param_types[i] == PARAM_NORMAL ||
-            function->param_types[i] == PARAM_ARRAY) {
-            if (direction == GI_DIRECTION_IN || direction == GI_DIRECTION_INOUT)
-                function->expected_js_argc += 1;
-            if (direction == GI_DIRECTION_OUT || direction == GI_DIRECTION_INOUT)
-                function->js_out_argc += 1;
+        if (direction == GI_DIRECTION_OUT || direction == GI_DIRECTION_INOUT) {
+            function->out_arguments[function->js_out_argc] = out_args[i];
+            function->js_out_argc++;
         }
     }
 
diff --git a/gjs-srcs.mk b/gjs-srcs.mk
index c7600bef..9b4c8a23 100644
--- a/gjs-srcs.mk
+++ b/gjs-srcs.mk
@@ -14,6 +14,8 @@ gjs_public_headers =          \
 gjs_srcs =                             \
        gi/arg.cpp                      \
        gi/arg.h                        \
+       gi/arg-cache.cpp                \
+       gi/arg-cache.h                  \
        gi/boxed.cpp                    \
        gi/boxed.h                      \
        gi/closure.cpp                  \


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]