Closed Bug 1054630 Opened 5 years ago Closed 5 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)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- wontfix
firefox34 --- affected

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(6 files, 1 obsolete file)

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
Attachment #8474082 - Flags: feedback?(till)
Blocks: 867612
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+
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)
Part 2: Collect telemetry on usage of SpiderMonkey's deprecated for-each.
Attachment #8476519 - Flags: review?(till)
Part 3: Collect telemetry on usage of SpiderMonkey's deprecated destructuring for-in.
Attachment #8476520 - Flags: review?(till)
Part 4: Collect telemetry on usage of SpiderMonkey's deprecated legacy generators
Attachment #8476521 - Flags: review?(till)
Part 5: Collect telemetry on usage of SpiderMonkey's deprecated expression closures (shorthand function syntax).
Attachment #8476522 - Flags: review?(till)
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)
Attachment #8476522 - Attachment description: 1054630_part-5-add-expression-closure-telemetry.patch → part-5-add-expression-closure-telemetry.patch
Attachment #8476524 - Attachment description: 1054630_part-6-add-addon-telemetry.patch → part-6-add-addon-telemetry.patch
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+
(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.
Attachment #8476519 - Flags: review?(till) → review+
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 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 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 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+
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
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
See Also: → 995610
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
(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?
https://hg.mozilla.org/mozilla-central/rev/efece5e33d82
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(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.
Depends on: 1060466
Blocks: 1083453
Blocks: 824289
Blocks: 1083485
Blocks: 1083497
Blocks: 1102131
No longer blocks: 1083485
You need to log in before you can comment on or make changes to this bug.