The default bug view has changed. See this FAQ.

add some initial telemetry counters to SpiderMonkey

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 2 obsolete attachments)

14.24 KB, patch
(dormant account)
: review+
Waldo
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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?
Attachment #544671 - Flags: feedback?(tglek)
(Assignee)

Comment 1

6 years ago
Heh, ignore the empty xpctelemetry.h file...

Comment 2

6 years ago
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
Attachment #544671 - Flags: feedback?(tglek) → feedback-
(Assignee)

Comment 3

6 years ago
(In reply to comment #2)
Sounds good
(Assignee)

Comment 4

6 years ago
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?
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #550273 - Flags: review?(tglek)
Attachment #550273 - Flags: review?(jwalden+bmo)

Comment 5

6 years ago
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.
Attachment #550273 - Flags: review?(tglek) → review-
(Assignee)

Comment 6

6 years ago
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

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
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 :)
Attachment #544671 - Attachment is obsolete: true
Attachment #550273 - Attachment is obsolete: true
Attachment #550441 - Flags: review?(tglek)
Attachment #550441 - Flags: review?(jwalden+bmo)
Attachment #550273 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

6 years ago
Good news: I browsed alexa's top 10 for 10 minutes and all three stats are 0 :)
Also i would like to make a few requests:
- RegExp.prototype.compile
- RegExp['$_'], ['$*'] etc.
- somehow measure how often we profit from COMPILE_N_GO
(Assignee)

Comment 11

6 years ago
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

6 years ago
Comment on attachment 550441 [details] [diff] [review]
patch 3

thanks
Attachment #550441 - Flags: review?(tglek) → review+
Attachment #550441 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/a22fad398472
Whiteboard: [inbound]
(Assignee)

Comment 14

6 years ago
Forgot to add a file, so backout
http://hg.mozilla.org/integration/mozilla-inbound/rev/706c47369f83
and reland
http://hg.mozilla.org/integration/mozilla-inbound/rev/ba19e1cd3f91
http://hg.mozilla.org/mozilla-central/rev/ba19e1cd3f91
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Depends on: 685373
You need to log in before you can comment on or make changes to this bug.