GNOME.org
 

[gjs: 10/11] profiler: Reduce API surface

commit c3a4f694e84ca002c7343158cdb56833a37e0859
Author: Philip Chimento <philip endlessm com>
Date:   Tue Jan 23 20:36:38 2018 -0800

    profiler: Reduce API surface
    
    We actually don't want callers to have control over the lifetime of the
    profiler object. It should be tied entirely to the GjsContext. This makes
    most of the profiler API private.
    
    Now, to control the profiler, you have two construct-only properties of
    GjsContext: profiler-enabled and profiler-sigusr2. profiler-sigusr2
    implies profiler-enabled, and does the work that
    gjs_profiler_setup_signals() would previously have accomplished. The
    GJS_PROFILER_ENABLED environment variable, if set, overrides the value of
    profiler-signal.
    
    By default when you call gjs_context_eval(), the profiler will be started
    and stopped automatically around the evaluation of the JS code. If you
    need to call gjs_context_eval() more than once, you probably only want to
    do that within one profiling session. For that case we provide the
    gjs_profiler_start() and gjs_profiler_stop() API, which will prevent the
    auto-starting and stopping.

 gjs/console.cpp        | 27 ++++++--------
 gjs/context.cpp        | 99 ++++++++++++++++++++++++++++++++++++++++++++------
 gjs/context.h          |  8 ++++
 gjs/profiler-private.h |  8 +++-
 gjs/profiler.cpp       | 89 +++++++++++++++++++++++++--------------------
 gjs/profiler.h         | 18 ---------
 test/gjs-tests.cpp     |  8 ++--
 7 files changed, 169 insertions(+), 88 deletions(-)
---
diff --git a/gjs/console.cpp b/gjs/console.cpp
index 455c29f..61e8c77 100644
--- a/gjs/console.cpp
+++ b/gjs/console.cpp
@@ -193,7 +193,6 @@ main(int argc, char **argv)
     GError *error = NULL;
     GjsContext *js_context;
     GjsCoverage *coverage = NULL;
-    GjsProfiler *profiler = nullptr;
     char *script;
     const char *filename;
     const char *program_name;
@@ -284,9 +283,16 @@ main(int argc, char **argv)
     /* This should be removed after a suitable time has passed */
     check_script_args_for_stray_gjs_args(script_argc, script_argv);
 
+    if (interactive_mode && enable_profiler) {
+        g_message("Profiler disabled in interactive mode.");
+        enable_profiler = false;
+        g_unsetenv("GJS_ENABLE_PROFILER");  /* ignore env var in eval() */
+    }
+
     js_context = (GjsContext*) g_object_new(GJS_TYPE_CONTEXT,
                                             "search-path", include_path,
                                             "program-name", program_name,
+                                            "profiler-enabled", enable_profiler,
                                             NULL);
 
     env_coverage_output_path = g_getenv("GJS_COVERAGE_OUTPUT");
@@ -311,20 +317,10 @@ main(int argc, char **argv)
         g_object_unref(output);
     }
 
-    const char *env_profiler = g_getenv("GJS_ENABLE_PROFILER");
-    if (env_profiler)
-        enable_profiler = true;
-
-    if (interactive_mode && enable_profiler) {
-        g_message("Profiler disabled in interactive mode.");
-    } else if (enable_profiler) {
-        profiler = gjs_profiler_new(js_context);
-
-        if (profile_output_path) {
-            gjs_profiler_set_filename(profiler, profile_output_path);
-            g_free(profile_output_path);
-        }
-        gjs_profiler_start(profiler);
+    if (enable_profiler && profile_output_path) {
+        GjsProfiler *profiler = gjs_context_get_profiler(js_context);
+        gjs_profiler_set_filename(profiler, profile_output_path);
+        g_free(profile_output_path);
     }
 
     /* prepare command line arguments */
@@ -357,7 +353,6 @@ main(int argc, char **argv)
     g_strfreev(coverage_prefixes);
     if (coverage)
         g_object_unref(coverage);
-    gjs_profiler_free(profiler);
     g_object_unref(js_context);
     g_free(script);
     exit(code);
diff --git a/gjs/context.cpp b/gjs/context.cpp
index cb34c86..6998ee0 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -93,6 +93,10 @@ struct _GjsContext {
     bool draining_job_queue;
 
     std::unordered_map<uint64_t, GjsAutoChar> unhandled_rejection_stacks;
+
+    GjsProfiler *profiler;
+    bool should_profile : 1;
+    bool should_listen_sigusr2 : 1;
 };
 
 /* Keep this consistent with GjsConstString */
@@ -118,6 +122,8 @@ enum {
     PROP_0,
     PROP_SEARCH_PATH,
     PROP_PROGRAM_NAME,
+    PROP_PROFILER_ENABLED,
+    PROP_PROFILER_SIGUSR2,
 };
 
 static GMutex contexts_lock;
@@ -164,6 +170,39 @@ gjs_context_class_init(GjsContextClass *klass)
                                     pspec);
     g_param_spec_unref(pspec);
 
+    /**
+     * GjsContext:profiler-enabled:
+     *
+     * Set this property to profile any JS code run by this context. By
+     * default, the profiler is started and stopped when you call
+     * gjs_context_eval().
+     *
+     * The value of this property is superseded by the GJS_ENABLE_PROFILER
+     * environment variable.
+     *
+     * You may only have one context with the profiler enabled at a time.
+     */
+    pspec = g_param_spec_boolean("profiler-enabled", "Profiler enabled",
+                                 "Whether to profile JS code run by this context",
+                                 FALSE,
+                                 GParamFlags(G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY));
+    g_object_class_install_property(object_class, PROP_PROFILER_ENABLED, pspec);
+    g_param_spec_unref(pspec);
+
+    /**
+     * GjsContext:profiler-sigusr2:
+     *
+     * Set this property to install a SIGUSR2 signal handler that starts and
+     * stops the profiler. This property also implies that
+     * #GjsContext:profiler-enabled is set.
+     */
+    pspec = g_param_spec_boolean("profiler-sigusr2", "Profiler SIGUSR2",
+                                 "Whether to activate the profiler on SIGUSR2",
+                                 FALSE,
+                                 GParamFlags(G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY));
+    g_object_class_install_property(object_class, PROP_PROFILER_SIGUSR2, pspec);
+    g_param_spec_unref(pspec);
+
     /* For GjsPrivate */
     {
 #ifdef G_OS_WIN32
@@ -214,6 +253,10 @@ gjs_context_dispose(GObject *object)
 
     js_context = GJS_CONTEXT(object);
 
+    /* Profiler must be stopped and freed before context is shut down */
+    if (js_context->profiler)
+        g_clear_pointer(&js_context->profiler, _gjs_profiler_free);
+
     /* Run dispose notifications first, so that anything releasing
      * references in response to this can still get garbage collected */
     G_OBJECT_CLASS(gjs_context_parent_class)->dispose(object);
@@ -307,6 +350,21 @@ gjs_context_constructed(GObject *object)
         g_error("Failed to create javascript context");
     js_context->context = cx;
 
+    const char *env_profiler = g_getenv("GJS_ENABLE_PROFILER");
+    if (env_profiler || js_context->should_listen_sigusr2)
+        js_context->should_profile = true;
+
+    if (js_context->should_profile) {
+        js_context->profiler = _gjs_profiler_new(js_context);
+
+        if (!js_context->profiler) {
+            js_context->should_profile = false;
+        } else {
+            if (js_context->should_listen_sigusr2)
+                _gjs_profiler_setup_signals(js_context->profiler, js_context);
+        }
+    }
+
     new (&js_context->unhandled_rejection_stacks) std::unordered_map<uint64_t, GjsAutoChar>;
     new (&js_context->const_strings) std::array<JS::PersistentRootedId*, GJS_STRING_LAST>;
     for (i = 0; i < GJS_STRING_LAST; i++) {
@@ -391,6 +449,12 @@ gjs_context_set_property (GObject      *object,
     case PROP_PROGRAM_NAME:
         js_context->program_name = g_value_dup_string(value);
         break;
+    case PROP_PROFILER_ENABLED:
+        js_context->should_profile = g_value_get_boolean(value);
+        break;
+    case PROP_PROFILER_SIGUSR2:
+        js_context->should_listen_sigusr2 = g_value_get_boolean(value);
+        break;
     default:
         G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
         break;
@@ -691,18 +755,19 @@ gjs_context_eval(GjsContext   *js_context,
 {
     bool ret = false;
 
-    GjsProfiler *profiler = nullptr;
-    const char *env_profiler = g_getenv("GJS_ENABLE_PROFILER");
-    if (env_profiler && !_gjs_profiler_get_current()) {
-        profiler = gjs_profiler_new(js_context);
-        gjs_profiler_start(profiler);
-    }
+    bool auto_profile = js_context->should_profile;
+    if (auto_profile && (_gjs_profiler_is_running(js_context->profiler) ||
+                         js_context->should_listen_sigusr2))
+        auto_profile = false;
 
     JSAutoCompartment ac(js_context->context, js_context->global);
     JSAutoRequest ar(js_context->context);
 
     g_object_ref(G_OBJECT(js_context));
 
+    if (auto_profile)
+        gjs_profiler_start(js_context->profiler);
+
     JS::RootedValue retval(js_context->context);
     bool ok = gjs_eval_with_scope(js_context->context, nullptr, script,
                                   script_len, filename, &retval);
@@ -712,8 +777,8 @@ gjs_context_eval(GjsContext   *js_context,
      * uncaught exceptions have been reported since draining runs callbacks. */
     ok = _gjs_context_run_jobs(js_context) && ok;
 
-    if (profiler)
-        gjs_profiler_stop(profiler);
+    if (auto_profile)
+        gjs_profiler_stop(js_context->profiler);
 
     if (!ok) {
         uint8_t code;
@@ -751,9 +816,6 @@ gjs_context_eval(GjsContext   *js_context,
     ret = true;
 
  out:
-    if (profiler)
-        gjs_profiler_free(profiler);
-
     g_object_unref(G_OBJECT(js_context));
     context_reset_exit(js_context);
     return ret;
@@ -856,3 +918,18 @@ gjs_get_import_global(JSContext *context)
     GjsContext *gjs_context = (GjsContext *) JS_GetContextPrivate(context);
     return gjs_context->global;
 }
+
+/**
+ * gjs_context_get_profiler:
+ * @self: the #GjsContext
+ *
+ * Returns the profiler's internal instance of #GjsProfiler for you to
+ * customize, or %NULL if profiling is not enabled on this #GjsContext.
+ *
+ * Returns: (transfer none) (nullable): a #GjsProfiler
+ */
+GjsProfiler *
+gjs_context_get_profiler(GjsContext *self)
+{
+    return self->profiler;
+}
diff --git a/gjs/context.h b/gjs/context.h
index f066a33..8a2df29 100644
--- a/gjs/context.h
+++ b/gjs/context.h
@@ -32,6 +32,7 @@
 #include <glib-object.h>
 
 #include <gjs/macros.h>
+#include <gjs/profiler.h>
 
 G_BEGIN_DECLS
 
@@ -91,6 +92,13 @@ void            gjs_context_maybe_gc              (GjsContext  *context);
 GJS_EXPORT
 void            gjs_context_gc                    (GjsContext  *context);
 
+GJS_EXPORT
+GjsProfiler *gjs_context_get_profiler(GjsContext *self);
+
+GJS_EXPORT
+bool gjs_profiler_chain_signal(GjsContext *context,
+                               siginfo_t  *info);
+
 GJS_EXPORT
 void            gjs_dumpstack                     (void);
 
diff --git a/gjs/profiler-private.h b/gjs/profiler-private.h
index 1e17de8..98bb722 100644
--- a/gjs/profiler-private.h
+++ b/gjs/profiler-private.h
@@ -24,11 +24,17 @@
 #ifndef GJS_PROFILER_PRIVATE_H
 #define GJS_PROFILER_PRIVATE_H
 
+#include "context.h"
 #include "profiler.h"
 
 G_BEGIN_DECLS
 
-GjsProfiler *_gjs_profiler_get_current(void);
+GjsProfiler *_gjs_profiler_new(GjsContext *context);
+void _gjs_profiler_free(GjsProfiler *self);
+
+bool _gjs_profiler_is_running(GjsProfiler *self);
+
+void _gjs_profiler_setup_signals(GjsProfiler *self, GjsContext *context);
 
 G_END_DECLS
 
diff --git a/gjs/profiler.cpp b/gjs/profiler.cpp
index 5049433..ac0be8a 100644
--- a/gjs/profiler.cpp
+++ b/gjs/profiler.cpp
@@ -37,6 +37,7 @@
 #include "jsapi-wrapper.h"
 #include <js/ProfilingStack.h>
 
+#include "context.h"
 #include "jsapi-util.h"
 #include "profiler-private.h"
 #ifdef ENABLE_PROFILER
@@ -106,23 +107,16 @@ struct _GjsProfiler {
 
     /* Cached copy of our pid */
     GPid pid;
+
+    /* GLib signal handler ID for SIGUSR2 */
+    unsigned sigusr2_id;
 #endif  /* ENABLE_PROFILER */
 
     /* If we are currently sampling */
     unsigned running : 1;
 };
 
-static GjsProfiler *current_profiler;
-
-/* Internal use only - used to determine whether a profiler is already created,
- * in order to accommodate code that links to libgjs but doesn't create its own
- * profiler while deciding what to do for the GJS_ENABLE_PROFILER=1 environment
- * variable. */
-GjsProfiler *
-_gjs_profiler_get_current(void)
-{
-    return current_profiler;
-}
+static GjsContext *profiling_context;
 
 #ifdef ENABLE_PROFILER
 /*
@@ -182,12 +176,12 @@ gjs_profiler_extract_maps(GjsProfiler *self)
 }
 #endif  /* ENABLE_PROFILER */
 
-/**
- * gjs_profiler_new:
+/*
+ * _gjs_profiler_new:
  * @context: The #GjsContext to profile
  *
  * This creates a new profiler for the #JSContext. It is important that
- * this instance is freed with gjs_profiler_free() before the context is
+ * this instance is freed with _gjs_profiler_free() before the context is
  * destroyed.
  *
  * Call gjs_profiler_start() to enable the profiler, and gjs_profiler_stop()
@@ -199,16 +193,26 @@ gjs_profiler_extract_maps(GjsProfiler *self)
  * sample and stash it away. It is a programming error to mask SIGPROF from
  * the thread controlling the JS context.
  *
- * Returns: (transfer full): A newly allocated #GjsProfiler
+ * If another #GjsContext already has a profiler, or @context already has one,
+ * then returns %NULL instead.
+ *
+ * Returns: (transfer full) (nullable): A newly allocated #GjsProfiler
  */
 GjsProfiler *
-gjs_profiler_new(GjsContext *context)
+_gjs_profiler_new(GjsContext *context)
 {
     g_return_val_if_fail(context, nullptr);
 
-    g_assert(((void)"You can ony create one profiler at a time.",
-              !current_profiler));
+    if (profiling_context == context) {
+        g_critical("You can only create one profiler at a time.");
+        return nullptr;
+    }
 
+    if (profiling_context) {
+        g_message("Not going to profile GjsContext %p; you can only profile "
+                  "one context at a time.", context);
+        return nullptr;
+    }
 
     GjsProfiler *self = g_new0(GjsProfiler, 1);
 
@@ -217,13 +221,13 @@ gjs_profiler_new(GjsContext *context)
     self->pid = getpid();
 #endif
 
-    current_profiler = self;
+    profiling_context = context;
 
     return self;
 }
 
-/**
- * gjs_profiler_free:
+/*
+ * _gjs_profiler_free:
  * @self: A #GjsProfiler
  *
  * Frees a profiler instance and cleans up any allocated data.
@@ -232,7 +236,7 @@ gjs_profiler_new(GjsContext *context)
  * to write the contents of the buffer to the underlying file-descriptor.
  */
 void
-gjs_profiler_free(GjsProfiler *self)
+_gjs_profiler_free(GjsProfiler *self)
 {
     if (!self)
         return;
@@ -240,7 +244,7 @@ gjs_profiler_free(GjsProfiler *self)
     if (self->running)
         gjs_profiler_stop(self);
 
-    current_profiler = nullptr;
+    profiling_context = nullptr;
 
     g_clear_pointer(&self->filename, g_free);
 #ifdef ENABLE_PROFILER
@@ -249,8 +253,8 @@ gjs_profiler_free(GjsProfiler *self)
     g_free(self);
 }
 
-/**
- * gjs_profiler_is_running:
+/*
+ * _gjs_profiler_is_running:
  * @self: A #GjsProfiler
  *
  * Checks if the profiler is currently running. This means that the JS
@@ -259,7 +263,7 @@ gjs_profiler_free(GjsProfiler *self)
  * Returns: %TRUE if the profiler is active.
  */
 bool
-gjs_profiler_is_running(GjsProfiler *self)
+_gjs_profiler_is_running(GjsProfiler *self)
 {
     g_return_val_if_fail(self, false);
 
@@ -288,7 +292,7 @@ gjs_profiler_sigprof(int        signum,
                      siginfo_t *info,
                      void      *unused)
 {
-    GjsProfiler *self = current_profiler;
+    GjsProfiler *self = gjs_context_get_profiler(profiling_context);
 
     g_assert(((void) "SIGPROF handler called with invalid signal info", info));
     g_assert(((void) "SIGPROF handler called with other signal",
@@ -507,10 +511,13 @@ gjs_profiler_stop(GjsProfiler *self)
 }
 
 static gboolean
-gjs_profiler_sigusr2(void *unused)
+gjs_profiler_sigusr2(void *data)
 {
+    auto context = static_cast<GjsContext *>(data);
+    GjsProfiler *current_profiler = gjs_context_get_profiler(context);
+
     if (current_profiler) {
-        if (gjs_profiler_is_running(current_profiler))
+        if (_gjs_profiler_is_running(current_profiler))
             gjs_profiler_stop(current_profiler);
         else
             gjs_profiler_start(current_profiler);
@@ -519,8 +526,9 @@ gjs_profiler_sigusr2(void *unused)
     return G_SOURCE_CONTINUE;
 }
 
-/**
- * gjs_profiler_setup_signals:
+/*
+ * _gjs_profiler_setup_signals:
+ * @context: a #GjsContext with a profiler attached
  *
  * If you want to simply allow profiling of your process with minimal
  * fuss, simply call gjs_profiler_setup_signals(). This will allow
@@ -532,16 +540,17 @@ gjs_profiler_sigusr2(void *unused)
  * own signal handler to pass the signal to a GjsProfiler.
  */
 void
-gjs_profiler_setup_signals(void)
+_gjs_profiler_setup_signals(GjsProfiler *self,
+                            GjsContext  *context)
 {
+    g_return_if_fail(context == profiling_context);
+
 #ifdef ENABLE_PROFILER
 
-    static bool initialized = false;
+    if (self->sigusr2_id != 0)
+        return;
 
-    if (!initialized) {
-        initialized = true;
-        g_unix_signal_add(SIGUSR2, gjs_profiler_sigusr2, nullptr);
-    }
+    self->sigusr2_id = g_unix_signal_add(SIGUSR2, gjs_profiler_sigusr2, context);
 
 #else  /* !ENABLE_PROFILER */
 
@@ -552,6 +561,7 @@ gjs_profiler_setup_signals(void)
 
 /**
  * gjs_profiler_chain_signal:
+ * @context: a #GjsContext with a profiler attached
  * @info: #siginfo_t passed in to signal handler
  *
  * Use this to pass a signal info caught by another signal handler to a
@@ -563,7 +573,8 @@ gjs_profiler_setup_signals(void)
  * Returns: %TRUE if the signal was handled.
  */
 bool
-gjs_profiler_chain_signal(siginfo_t *info)
+gjs_profiler_chain_signal(GjsContext *context,
+                          siginfo_t *info)
 {
 #ifdef ENABLE_PROFILER
 
@@ -574,7 +585,7 @@ gjs_profiler_chain_signal(siginfo_t *info)
         }
 
         if (info->si_signo == SIGUSR2) {
-            gjs_profiler_sigusr2(nullptr);
+            gjs_profiler_sigusr2(context);
             return true;
         }
     }
diff --git a/gjs/profiler.h b/gjs/profiler.h
index f0fd971..8bebed2 100644
--- a/gjs/profiler.h
+++ b/gjs/profiler.h
@@ -24,9 +24,6 @@
 #ifndef GJS_PROFILER_H
 #define GJS_PROFILER_H
 
-#include <signal.h>
-#include <stdbool.h>
-
 #include <glib.h>
 
 #include <gjs/context.h>
@@ -40,12 +37,6 @@ typedef struct _GjsProfiler GjsProfiler;
 GJS_EXPORT
 GType gjs_profiler_get_type(void);
 
-GJS_EXPORT
-GjsProfiler *gjs_profiler_new(GjsContext *context);
-
-GJS_EXPORT
-void gjs_profiler_free(GjsProfiler *self);
-
 GJS_EXPORT
 void gjs_profiler_set_filename(GjsProfiler *self,
                                const char  *filename);
@@ -56,15 +47,6 @@ void gjs_profiler_start(GjsProfiler *self);
 GJS_EXPORT
 void gjs_profiler_stop(GjsProfiler *self);
 
-GJS_EXPORT
-bool gjs_profiler_is_running(GjsProfiler *self);
-
-GJS_EXPORT
-void gjs_profiler_setup_signals(void);
-
-GJS_EXPORT
-bool gjs_profiler_chain_signal(siginfo_t *info);
-
 G_END_DECLS
 
 #endif /* GJS_PROFILER_H */
diff --git a/test/gjs-tests.cpp b/test/gjs-tests.cpp
index 24f395c..cc4b603 100644
--- a/test/gjs-tests.cpp
+++ b/test/gjs-tests.cpp
@@ -379,8 +379,11 @@ gjstest_test_strip_shebang_return_null_for_just_shebang(void)
 static void
 gjstest_test_profiler_start_stop(void)
 {
-    GjsAutoUnref<GjsContext> context = gjs_context_new();
-    GjsProfiler *profiler = gjs_profiler_new(context);
+    GjsAutoUnref<GjsContext> context =
+        static_cast<GjsContext *>(g_object_new(GJS_TYPE_CONTEXT,
+                                               "profiler-enabled", TRUE,
+                                               nullptr));
+    GjsProfiler *profiler = gjs_context_get_profiler(context);
 
     gjs_profiler_start(profiler);
 
@@ -397,7 +400,6 @@ gjstest_test_profiler_start_stop(void)
     }
 
     gjs_profiler_stop(profiler);
-    gjs_profiler_free(profiler);
 }
 
 int
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]