This patch adds plumbing for parser telemetry and then tracks destructuring for-in, which was added in JS 1.7 and removed in JS 1.8. We can add telemetry for other nonstandard language extensions, like for-each, legacy generators, and expression closures (shorthand function syntax), but I wanted to start with something simple. Unlike those other language extensions, AFAICT, destructuring for-in does not seem to be used in chrome or add-ons: https://mxr.mozilla.org/mozilla-central/search?string=for+*\%28%28var|let%29%3F+*\[&regexp=1 https://mxr.mozilla.org/addons/search?string=for+*\%28%28var|let%29%3F+*\[&regexp=1

Comment on attachment 8474082 [details] [diff] [review] telemetry-destructuring-for-in.patch Review of attachment 8474082 [details] [diff] [review]: ----------------------------------------------------------------- How delightfully simple, and thank you so much for doing this! Patch looks very good, so switching to r+. (I don't know if we need a review from a data collection peer, but IIUC, that's only needed for FHR-type stuff, right?) I think I'd ever so slightly prefer for this to be split into two patches, one for the infrastructure one for the first probe, but then again I don't think it matters too much. Ideally, we'd land probes for all the extensions in one Firefox version, so we don't get weirdly-skewed data. ::: js/src/frontend/Parser.cpp @@ +7532,5 @@ > +{ > + JSContext* cx = context->maybeJSContext(); > + if (!cx) > + return; > + JSRuntime* rt = GetRuntime(cx); `cx->runtime()`, and I think you can just inline this to `cb = cx->runtime()-telemetryCallback`. ::: js/src/frontend/Parser.h @@ +349,5 @@ > > /* Unexpected end of input, i.e. TOK_EOF not at top-level. */ > bool isUnexpectedEOF_:1; > > + /* Collect telemetry on SpiderMonkey's nonstandard language extensions. */ Perhaps "Used for collecting ..."? ::: js/src/jsfriendapi.h @@ +113,5 @@ > JS_TELEMETRY_GC_INCREMENTAL_DISABLED, > JS_TELEMETRY_GC_NON_INCREMENTAL, > JS_TELEMETRY_GC_SCC_SWEEP_TOTAL_MS, > + JS_TELEMETRY_GC_SCC_SWEEP_MAX_PAUSE_MS, > + JS_TELEMETRY_LANGUAGE_EXTENSIONS, Maybe "DEPRECATED_LANGUAGE_EXTENSIONS"? (But then again, there's no other kind, so it doesn't really matter. Makes it a bit more self-explanatory why this is here, though.)

Part 1: Add plumbing for SpiderMonkey parser telemetry. Add parser telemetry support in its own patch, followed by patches for each deprecated language extension, and then add-on telemetry.

Part 2: Collect telemetry on usage of SpiderMonkey's deprecated for-each.

Part 3: Collect telemetry on usage of SpiderMonkey's deprecated destructuring for-in.

Part 4: Collect telemetry on usage of SpiderMonkey's deprecated legacy generators

Part 5: Collect telemetry on usage of SpiderMonkey's deprecated expression closures (shorthand function syntax).

Part 6: Collect telemetry on usage of SpiderMonkey's deprecated language extensions in add-on JS. Note that, as we discussed in email, add-on telemetry should not land until use of deprecated language extensions are removed the Add-on SDK.

Comment on attachment 8476518 [details] [diff] [review] part-1-add-parser-telemetry.patch Review of attachment 8476518 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the callback actually invoked. ::: js/src/frontend/Parser.cpp @@ +7582,5 @@ > + > + const char* filename = getFilename(); > + bool isHTTP = strncmp(filename, "http://", 7) == 0 || strncmp(filename, "https://", 8) == 0; > + > + // Only report telemetry for web content, not add-ons or chrome JS. Maybe file a follow-up bug for adding add-ons telemetry once the SDK has been cleaned up, and then add a FIXME pointing to that bug here? @@ +7584,5 @@ > + bool isHTTP = strncmp(filename, "http://", 7) == 0 || strncmp(filename, "https://", 8) == 0; > + > + // Only report telemetry for web content, not add-ons or chrome JS. > + if (!isHTTP) > + return; Ah, I hadn't considered it a possibility to land reporting for web content right now. Makes a lot of sense. :) @@ +7586,5 @@ > + // Only report telemetry for web content, not add-ons or chrome JS. > + if (!isHTTP) > + return; > + > + // Call back into Firefox's Telemetry reporter. Ideally, this would really happen :) ::: js/src/jsfriendapi.h @@ +113,5 @@ > JS_TELEMETRY_GC_INCREMENTAL_DISABLED, > JS_TELEMETRY_GC_NON_INCREMENTAL, > JS_TELEMETRY_GC_SCC_SWEEP_TOTAL_MS, > + JS_TELEMETRY_GC_SCC_SWEEP_MAX_PAUSE_MS, > + JS_TELEMETRY_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT, I think the trailing comma should be removed.

(In reply to Till Schneidereit [:till] from comment #8) > r=me with the callback actually invoked. And, of course, that comment can be ignored, as you'll invoke it in the consecutive patches that actually add probes ...

Part 1: Add plumbing for SpiderMonkey parser telemetry. https://hg.mozilla.org/integration/mozilla-inbound/rev/f2da252a9248

Comment on attachment 8476519 [details] [diff] [review] part-2-add-for-each-telemetry.patch Review of attachment 8476519 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 8476524 [details] [diff] [review] part-6-add-addon-telemetry.patch Review of attachment 8476524 [details] [diff] [review]: ----------------------------------------------------------------- Yup. The file name threw me off, but as long as the description makes sense, that doesn't matter, I guess.

Comment on attachment 8476520 [details] [diff] [review] part-3-add-destructuring-for-in-telemetry.patch Review of attachment 8476520 [details] [diff] [review]: ----------------------------------------------------------------- Yes.

Comment on attachment 8476521 [details] [diff] [review] part-4-add-legacy-generator-telemetry.patch Review of attachment 8476521 [details] [diff] [review]: ----------------------------------------------------------------- Absolutely ::: js/src/frontend/Parser.cpp @@ +4801,4 @@ > // We are in code that has not seen a yield, but we are in JS 1.7 or > // later. Try to transition to being a legacy generator. > + JSVersion version = versionNumber(); > + JS_ASSERT(version >= JSVERSION_1_7); Not sure I understand this change, but if it's a cleanup, by all means.

Comment on attachment 8476522 [details] [diff] [review] part-5-add-expression-closure-telemetry.patch Review of attachment 8476522 [details] [diff] [review]: ----------------------------------------------------------------- Again, thanks for doing this.

Looks like part 1 was backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dca7432dca80 for causing crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=46546274&tree=Mozilla-Inbound (Bug 1036781 Part 4 was also backed out, but this probably caused them given the crashes appear parser related)

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #16) > Looks like part 1 was backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/dca7432dca80 > for causing crashes like > https://tbpl.mozilla.org/php/getParsedLog.php?id=46546274&tree=Mozilla- > Inbound oops! Apparently Parser can have a null filename. (In reply to Till Schneidereit [:till] from comment #12) > part-6-add-addon-telemetry.patch > > Yup. The file name threw me off, but as long as the description makes sense, > that doesn't matter, I guess. Sorry. That was a copy/paste error. (In reply to Till Schneidereit [:till] from comment #14) > ::: js/src/frontend/Parser.cpp > @@ +4801,4 @@ > > // We are in code that has not seen a yield, but we are in JS 1.7 or > > // later. Try to transition to being a legacy generator. > > + JSVersion version = versionNumber(); > > + JS_ASSERT(version >= JSVERSION_1_7); > > Not sure I understand this change, but if it's a cleanup, by all means. I reverted this JSVersion change locally. It was a leftover from an earlier revision of this patch that tried to detect chrome JS by checking for JSVERSION_ECMA_5 (instead of checking the filename prefix for "http://" or "chrome://").

https://hg.mozilla.org/integration/mozilla-inbound/rev/be39ba3aa672 https://hg.mozilla.org/integration/mozilla-inbound/rev/0bceb044a4de https://hg.mozilla.org/integration/mozilla-inbound/rev/19d115a44912 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa6d52a0cfea https://hg.mozilla.org/integration/mozilla-inbound/rev/e656c61ecd70 https://hg.mozilla.org/integration/mozilla-inbound/rev/684b95601155

This push seems to have busted hazards. Do you want to take a look or would you like me to back it out?

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/14cf1ce4b667 for hazards bustage. till took a quick look, but couldn't figure out what was wrong and recommended I back out.

The Analysis doesn't know that the telemetry callback won't GC. (I assume it doesn't) https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-br-haz/20140824004153/hazards.txt.gz Easiest fix is probably to add "JS::AutoSuppressGCAnalysis nogc;" to accumulateTelemetry.

Thanks, Nigel and Tom. The telemetry callbacks just insert some counters into a hash table, so I don't think they GC. Here's a green try build with JS::AutoSuppressGCAnalysis: https://tbpl.mozilla.org/?tree=Try&rev=6a0b783e0a62

OK, third time's the charm: https://hg.mozilla.org/integration/mozilla-inbound/rev/efece5e33d82

(In reply to Chris Peterson (:cpeterson) from comment #22) > Thanks, Nigel and Tom. The telemetry callbacks just insert some counters > into a hash table, so I don't think they GC. Here's a green try build with > JS::AutoSuppressGCAnalysis: Hum, will the hash table potentially be resized during this? If so, it might well trigger a GC. Can you verify with GC people?

(In reply to Till Schneidereit [:till] from comment #24) > Hum, will the hash table potentially be resized during this? If so, it might > well trigger a GC. Can you verify with GC people? The telemetry code uses NSPR's PLDHashTable (which resizes using malloc) and Chromium's base/histogram (which uses std::vector). Terrence says these can never trigger a GC.

Ok, great. Thanks for verifying.