Created attachment 156268 [details] [review]
[WIP] Add ShellMetrics

This is a now-ancient patch that I never filed.  Putting it
in Bugzilla so it doesn't get lost.

Created attachment 156525 [details] [review]
[ShellMetrics] New class for tracking data

Currently, this class is just used to snapshot memory-usage
related statistics.  However, we could easily extend it to
have say number of times the overview is opened, count of
open windows, etc.

Review of attachment 156525 [details] [review]:

I didn't review the HTML file...

::: data/Makefile.am
@@ +50,3 @@
+metricsdir = $(pkgdatadir)
+metrics_DATA =              \
+    metrics.html

need a tab, not spaces

@@ +57,3 @@
 EXTRA_DIST =					\
 	gnome-shell.desktop.in.in		\
+    $(metrics_DATA)             \

likewise

::: src/gnome-shell.in
@@ +73,3 @@
+    shell_proxy.Eval("""
+const Gio = imports.gi.Gio;
+let metrics = Shell.Metrics.get_default();

you could indent the eval code under the dbus call anyway. the extra spaces will just be ignored...

@@ +95,3 @@
+    (tempfd, temppath) = tempfile.mkstemp(".html", "gnome-shell-metrics")
+    os.close(tempfd)
+    f = open(temppath, 'w')

boo! it's almost acceptable to misuse mkstemp in acquire_metrics() since you need to know the filename on both sides of the dbus connection. But there's no reason for it here.

@@ -127,3 @@
 def start_shell():
-    bin_dir = os.path.dirname(os.path.abspath(sys.argv[0]))
-    if os.path.exists(os.path.join(bin_dir, 'gnome-shell.in')):

not sure why you moved this to the toplevel? it doesn't seem to be used there

@@ -369,2 +399,3 @@
     result = shell.Eval(contents)
     print result
+

this appears to drop the "sys.exit(0)" from the "eval_file" branch

::: src/shell-global.c
@@ +207,3 @@
+                             "arena",
+                             "uordblks",
+                             "hblkhd",

might be nice to have a description/display name of what each stat means too, which would eventually end up in the HTML output.

@@ +307,3 @@
+  struct mallinfo info;
+
+  info = mallinfo ();

you probably need a configure test for <malloc.h> and mallinfo(). Google suggests Solaris has it but BSD doesn't.

::: src/shell-metrics.c
@@ +65,3 @@
+
+static long
+get_time (void)

this is just equivalent to calling "time (NULL)"

@@ +144,3 @@
+
+static void
+check_for_delta (ShellMetrics *metrics,

not the best name. "maybe_add_snapshot"?

@@ +158,3 @@
+
+  curvalue = *curvaluep;
+  was_nan = isnan(curvalue);

space before (

@@ +169,3 @@
+    return;
+
+  g_printerr ("metric %s; value:%f was_nan: %d curvaluep:%f, delta:%f\n", canon_id, value, was_nan, *curvaluep, delta);

leftover debug stuff? (ftr note that g_printerr mallocs)

@@ +210,3 @@
+      shell_metrics_freeze (metrics);
+
+      shell_metrics_update_prefixed (metrics, prefix, id, value);

a little odd. could just have a conditional call to shell_metrics_freeze(), then the body of the function, then a conditional call to shell_metrics_thaw(). or have freeze/thaw keep a counter

@@ +249,3 @@
+                               const char     *prefix,
+                               const char     *id,
+                               ShellMetricType type)

the standard gtk style (not sure if it's part of gnu style or our local variation?) always has at least one completely blank column between the types and variable names (or equivalently, it reserves room for the same number of "*"s on every variable). (Also affects shell_metrics_define(), shell_metrics_define_many(), shell_metrics_add_probe(), require_object_property_chain_recurse(), and shell_metrics_dump().)

@@ +359,3 @@
+  guint i, item_offset;
+
+

extra blank line here

I am going to trust that you got the json-glib memory management correct, or else that if you didn't, that your framework will detect its own leaks. :-)

@@ +425,3 @@
+  generator = metrics_to_json (metrics);
+  buf = json_generator_to_data (generator, &len);
+  g_object_unref (generator);

any reason metrics_to_json returns a generator rather than just having it do the json_generator_to_data() call too?

@@ +439,3 @@
+      g_source_remove (metrics->snapshot_id);
+      metrics->snapshot_id = 0;
+    }

need to chain up to parent dispose

@@ +443,3 @@
+
+static void
+shell_metrics_class_init(ShellMetricsClass *klass)

space before (

@@ +472,3 @@
+ */
+ShellMetrics *
+shell_metrics_get_default ()

(void)

::: src/shell-metrics.h
@@ +33,3 @@
+typedef void (*ShellMetricsProbe)(ShellMetrics *metrics, gpointer user_data);
+
+ShellMetrics* shell_metrics_get_default(void);

space before (

it would be nice to line up the declarations here

Would be nice to have some design background about what you were thinking, about how this fits into a big picture.

It seems that what you've created here is a start of a "performance event log". In my opinion regular time measurements of global parameters like memory usage are one part of that, but there's also room for other things in the log that don't fit that model.

 - We might want markers - at this point, the user triggered the overview, a window was opened, a frame was drawn, etc.
 - We might want a scripting framework to be able to trigger collection of a snapshot of measurements
 - We might want markers inserted at arbitrary points from a scripting framework.

I don't have too much comments on the details at this point beyond what Dan commented. The 'prefix' thing seems overcomplicated to me - a flat namespace for measurements would seem fine. The delta-compression also seems like a over-complication. In particular:

 +#define MIN_DELTA 0.01

Seems problematical, since it forces all measurements to have the same overall scale.

A performance event log is a very useful tool for investigating performance interactively - see bootchart or what we did with the Mugshot server. But interactive is not the full story and doesn't really help us getting comparative number across hardware or across versions of the shell.

Is the best way to get single numbers to script a series of actions and then crunch numbers out of the recorded event logs?

is this at all still relevant given the *other* perf framework we have now?

the perf framework was created as an alternative to this