1054630 - Collect telemetry on usage of SpiderMonkey's deprecated language extensions: for-each, destructuring for-in, legacy generators, and expression closures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: telemetry-destructuring-for-in.patch

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 1

[Till Schneidereit [:till]]

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

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-1-add-parser-telemetry.patch

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.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-2-add-for-each-telemetry.patch

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

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-3-add-destructuring-for-in-telemetry.patch

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

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: part-4-add-legacy-generator-telemetry.patch

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

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: part-5-add-expression-closure-telemetry.patch

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

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: part-6-add-addon-telemetry.patch

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 8

[Till Schneidereit [:till]]

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.

Comment 9

[Till Schneidereit [:till]]

(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 ...

Comment 10

[Chris Peterson [:cpeterson]]

Part 1: Add plumbing for SpiderMonkey parser telemetry.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2da252a9248

Comment 11

[Till Schneidereit [:till]]

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

Review of attachment 8476519 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 12

[Till Schneidereit [:till]]

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 13

[Till Schneidereit [:till]]

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

Review of attachment 8476520 [details] [diff] [review]:
-----------------------------------------------------------------

Yes.

Comment 14

[Till Schneidereit [:till]]

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 15

[Till Schneidereit [:till]]

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.

Comment 16

[Emanuel Hoogeveen [:ehoogeveen]]

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)

Comment 17

[Chris Peterson [:cpeterson]]

(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://").

Comment 18

[Chris Peterson [:cpeterson]]

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

Comment 19

[Nigel Babu [:nigelb]]

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

[Nigel Babu [:nigelb]]

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

[Tom Schuster [:evilpie]]

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.

Comment 22

[Chris Peterson [:cpeterson]]

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

Comment 23

[Chris Peterson [:cpeterson]]

OK, third time's the charm:

https://hg.mozilla.org/integration/mozilla-inbound/rev/efece5e33d82

Comment 24

[Till Schneidereit [:till]]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/efece5e33d82

Comment 26

[Chris Peterson [:cpeterson]]

(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

[Till Schneidereit [:till]]

Ok, great. Thanks for verifying.