Luke Wagner [:luke] Assignee
	Description • 13 years ago

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?

	Luke Wagner [:luke] Assignee
	Comment 1 • 13 years ago

Heh, ignore the empty xpctelemetry.h file...

	(dormant account)
	Comment 2 • 13 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

	Luke Wagner [:luke] Assignee
	Comment 3 • 13 years ago

(In reply to comment #2)
Sounds good

	Luke Wagner [:luke] Assignee
	Comment 4 • 13 years ago

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?

	(dormant account)
	Comment 5 • 13 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.

	Luke Wagner [:luke] Assignee
	Comment 6 • 13 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.

	(dormant account)
	Comment 7 • 13 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.

	Luke Wagner [:luke] Assignee
	Comment 8 • 13 years ago

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 :)

	Luke Wagner [:luke] Assignee
	Comment 9 • 13 years ago

Good news: I browsed alexa's top 10 for 10 minutes and all three stats are 0 :)

	Tom S [:evilpie]
	Comment 10 • 13 years ago

Also i would like to make a few requests:
- RegExp.prototype.compile
- RegExp['$_'], ['$*'] etc.
- somehow measure how often we profit from COMPILE_N_GO

	Luke Wagner [:luke] Assignee
	Comment 11 • 13 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)?

	(dormant account)
	Comment 12 • 13 years ago

Comment on attachment 550441 [details] [diff] [review]
patch 3

thanks

	Luke Wagner [:luke] Assignee
	Comment 13 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/a22fad398472

	Luke Wagner [:luke] Assignee
	Comment 14 • 13 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

	Marco Bonardo [:mak]
	Comment 15 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/ba19e1cd3f91