Last Comment Bug 670059 - add some initial telemetry counters to SpiderMonkey
: add some initial telemetry counters to SpiderMonkey
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Luke Wagner [:luke]
:
:
Mentors:
Depends on: 685373
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-07 18:19 PDT by Luke Wagner [:luke]
Modified: 2011-09-29 08:53 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (16.50 KB, patch)
2011-07-07 18:19 PDT, Luke Wagner [:luke]
taras.mozilla: feedback-
Details | Diff | Splinter Review
new patch (16.59 KB, patch)
2011-08-02 18:21 PDT, Luke Wagner [:luke]
taras.mozilla: review-
Details | Diff | Splinter Review
patch 3 (14.24 KB, patch)
2011-08-03 11:40 PDT, Luke Wagner [:luke]
taras.mozilla: review+
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-07-07 18:19:53 PDT
Created attachment 544671 [details] [diff] [review]
patch

Telemetry makes it easy to maintain a histogram.  What I'd like, though, is a simple counter that gets included with telemetry pings.  The attached does this and measures counts of three things:
 - number of times __iterator__ is used (i.e., is found for a for-in loop)
 - number of times __proto__ is set
 - number of E4X objects created

In each case, the interesting thing is content, so the counters use the fancy new "rt->trustedPrincipals" to ignore chrome.

The patch seems to work.  Does this seem like a reasonable short-term thing to add?
Comment 1 Luke Wagner [:luke] 2011-07-07 18:22:03 PDT
Heh, ignore the empty xpctelemetry.h file...
Comment 2 (dormant account) 2011-07-08 10:37:42 PDT
Comment on attachment 544671 [details] [diff] [review]
patch

Did you mean to include nsISimpleMeasurement.idl in the patch?

+  ret.js = Cc["@mozilla.org/js/xpc/XPConnect;1"]
+           .getService(Ci.nsISimpleMeasurement)
+           .value;

I would rather keep simplemeasurements as a flat structure. Can do
let js = Cc["@mozilla.org/js/xpc/XPConnect;1"]
           .getService(Ci.nsISimpleMeasurement)
           .value;
for (value in js) {
ret["js_"+value]=js[value]
} or something of the sort
Comment 3 Luke Wagner [:luke] 2011-07-08 10:51:14 PDT
(In reply to comment #2)
Sounds good
Comment 4 Luke Wagner [:luke] 2011-08-02 18:21:33 PDT
Created attachment 550273 [details] [diff] [review]
new patch

The way I tested is to enter this in a Chrome JS console:

  Components.classes["@mozilla.org/base/telemetry-ping;1"].createInstance(Components.interfaces.nsIObserver).observe(null, "test-ping", "http://localhost:12345")

nc -l 12345 produces (with extraneous text cut out at the ...):

  {"ver":1, ... "simpleMeasurements":{"uptime":10,"main":13,"firstPaint":7030,"sessionRestored":6394,"js_e4x":0,"js_setProto":0,"js_customIter":0}, ... }

I've also manually tested these three categories to see that they actually report uses as expected.

r?waldo for the JS engine bits and r?taras for the telemetry bits.  Taras, what additional steps are required, after review, to land?
Comment 5 (dormant account) 2011-08-02 19:40:33 PDT
Comment on attachment 550273 [details] [diff] [review]
new patch

I would r+ this, but I can't. nsISimpleMeasurement is the most ambiguous interface ever.

a) nsISimpleMeasurement.value has no comment
b) .value is actually a set js values.

Why not call it something descriptive like .jsEngineStats? It needs to be doc-commented accordingly

I'm guessing you don't root obj in GetValue because we have conservative stack scanning now?

+
+  var xpcRet = Cc["@mozilla.org/js/xpc/XPConnect;1"]
+               .getService(Ci.nsISimpleMeasurement)
+               .value;
+  for (let i in xpcRet)
+    ret["js_" + i] = xpcRet[i];
I know I originally suggested the .js_ convention, but now I think it might be better to nest the js stats.

ret.js =  Cc["@mozilla.org/js/xpc/XPConnect;1"]
               .getService(Ci.nsISimpleMeasurement).jsEngineStats

Once you get r+, land it. Our privacy policy covers this.
Comment 6 Luke Wagner [:luke] 2011-08-02 21:48:11 PDT
Yes it is the most ambiguous interface ever but this seems inherent: the sole purpose of the interface is to let custom-coded JS call custom-coded C++ -- there is no polymorphism or abstraction here.

As for .value, it started as a value and your last comment seems to indicate you want it to be put back to a value.  I think putting a jsEngineStats property on nsISimpleMeasurent would be a mistake since if we ever add some xyzStats property, then we'll need to define GetXYZStats on nsXPConnect as well as define GetJSEngineStats on xyz.

The only real option I see is to make a custom nsIJSEngineStats (defined in xpconnect) and not have any generic nsISimpleMeasurement.  I initially considered this and, while it does let the interface actually have semantics, it seemed like overkill.  Your code so your choice, though.  I just wanted to explain my thinking.
Comment 7 (dormant account) 2011-08-03 08:39:43 PDT
(In reply to comment #6)
> The only real option I see is to make a custom nsIJSEngineStats (defined in
> xpconnect) and not have any generic nsISimpleMeasurement.  I initially
> considered this and, while it does let the interface actually have
> semantics, it seemed like overkill.  Your code so your choice, though.  I
> just wanted to explain my thinking.

I suspect that other mozilla code can report stats in a simpler/saner way. So this is only needed for JS, having own nsIJSEngineStats sounds good.
Comment 8 Luke Wagner [:luke] 2011-08-03 11:40:41 PDT
Created attachment 550441 [details] [diff] [review]
patch 3

Done.  The simpleMeasurements subset of the ping blob now look like:

"simpleMeasurements":{"uptime":2,"main":7,"firstPaint":7229,"sessionRestored":6501,"js":{"e4x":5,"setProto":0,"customIter":0}}

Don't worry, that's after I manually created E4X objects :)
Comment 9 Luke Wagner [:luke] 2011-08-03 11:53:14 PDT
Good news: I browsed alexa's top 10 for 10 minutes and all three stats are 0 :)
Comment 10 AWAY Tom Schuster [:evilpie] 2011-08-03 12:07:49 PDT
Also i would like to make a few requests:
- RegExp.prototype.compile
- RegExp['$_'], ['$*'] etc.
- somehow measure how often we profit from COMPILE_N_GO
Comment 11 Luke Wagner [:luke] 2011-08-03 12:14:06 PDT
Should be easy; but how about you file a new bug depending on this and put up a new patch (on top of the one in this bug)?
Comment 12 (dormant account) 2011-08-03 12:24:45 PDT
Comment on attachment 550441 [details] [diff] [review]
patch 3

thanks
Comment 15 Marco Bonardo [::mak] 2011-08-05 09:09:10 PDT
http://hg.mozilla.org/mozilla-central/rev/ba19e1cd3f91

Note You need to log in before you can comment on or make changes to this bug.