Closed
Bug 1054630
Opened 7 years ago
Closed 7 years ago
Collect telemetry on usage of SpiderMonkey's deprecated language extensions: for-each, destructuring for-in, legacy generators, and expression closures
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(6 files, 1 obsolete file)
5.04 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
4.74 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
5.08 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
6.09 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
5.44 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
4.10 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
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+*\[®exp=1 https://mxr.mozilla.org/addons/search?string=for+*\%28%28var|let%29%3F+*\[®exp=1
Attachment #8474082 -
Flags: feedback?(till)
Comment 1•7 years ago
|
||
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.)
Attachment #8474082 -
Flags: feedback?(till) → review+
Assignee | ||
Comment 2•7 years ago
|
||
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.
Attachment #8474082 -
Attachment is obsolete: true
Attachment #8476518 -
Flags: review?(till)
Assignee | ||
Comment 3•7 years ago
|
||
Part 2: Collect telemetry on usage of SpiderMonkey's deprecated for-each.
Attachment #8476519 -
Flags: review?(till)
Assignee | ||
Comment 4•7 years ago
|
||
Part 3: Collect telemetry on usage of SpiderMonkey's deprecated destructuring for-in.
Attachment #8476520 -
Flags: review?(till)
Assignee | ||
Comment 5•7 years ago
|
||
Part 4: Collect telemetry on usage of SpiderMonkey's deprecated legacy generators
Attachment #8476521 -
Flags: review?(till)
Assignee | ||
Comment 6•7 years ago
|
||
Part 5: Collect telemetry on usage of SpiderMonkey's deprecated expression closures (shorthand function syntax).
Attachment #8476522 -
Flags: review?(till)
Assignee | ||
Comment 7•7 years ago
|
||
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.
Attachment #8476524 -
Flags: review?(till)
Assignee | ||
Updated•7 years ago
|
Attachment #8476522 -
Attachment description: 1054630_part-5-add-expression-closure-telemetry.patch → part-5-add-expression-closure-telemetry.patch
Assignee | ||
Updated•7 years ago
|
Attachment #8476524 -
Attachment description: 1054630_part-6-add-addon-telemetry.patch → part-6-add-addon-telemetry.patch
Comment 8•7 years ago
|
||
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.
Attachment #8476518 -
Flags: review?(till) → review+
Comment 9•7 years ago
|
||
(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 ...
Assignee | ||
Comment 10•7 years ago
|
||
Part 1: Add plumbing for SpiderMonkey parser telemetry. https://hg.mozilla.org/integration/mozilla-inbound/rev/f2da252a9248
Comment 11•7 years ago
|
||
Comment on attachment 8476519 [details] [diff] [review] part-2-add-for-each-telemetry.patch Review of attachment 8476519 [details] [diff] [review]: ----------------------------------------------------------------- Nice.
Attachment #8476519 -
Flags: review?(till) → review+
Comment 12•7 years ago
|
||
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.
Attachment #8476524 -
Flags: review?(till) → review+
Comment 13•7 years ago
|
||
Comment on attachment 8476520 [details] [diff] [review] part-3-add-destructuring-for-in-telemetry.patch Review of attachment 8476520 [details] [diff] [review]: ----------------------------------------------------------------- Yes.
Attachment #8476520 -
Flags: review?(till) → review+
Comment 14•7 years ago
|
||
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.
Attachment #8476521 -
Flags: review?(till) → review+
Comment 15•7 years ago
|
||
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.
Attachment #8476522 -
Flags: review?(till) → review+
Comment 16•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 years ago
|
||
(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://").
Assignee | ||
Comment 18•7 years ago
|
||
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
Keywords: leave-open
Summary: Collect telemetry on usage of SpiderMonkey's nonstandard destructuring for-in language extension → Collect telemetry on usage of SpiderMonkey's deprecated language extensions: for-each, destructuring for-in, legacy generators, and expression closures
Comment 19•7 years ago
|
||
This push seems to have busted hazards. Do you want to take a look or would you like me to back it out?
Comment 20•7 years ago
|
||
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.
Comment 21•7 years ago
|
||
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.
Assignee | ||
Comment 22•7 years ago
|
||
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
Assignee | ||
Comment 23•7 years ago
|
||
OK, third time's the charm: https://hg.mozilla.org/integration/mozilla-inbound/rev/efece5e33d82
Comment 24•7 years ago
|
||
(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?
Comment 25•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/efece5e33d82
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Assignee | ||
Comment 26•7 years ago
|
||
(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.
Comment 27•7 years ago
|
||
Ok, great. Thanks for verifying.
You need to log in
before you can comment on or make changes to this bug.
Description
•