988386 - Add telemetry for JS 1.5 - 1.8 version opt-in

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

One thing we need to be able to ever remove our proprietary language extensions is insight into how much they're used on the web. We know that they're used extensively by addons and chrome JS, but we're mostly assuming that that's not the case for web JS. It'd be nice to know if that holds.

As a first approximation, just knowing if a <script> node contains one of the relevant language version specifications would be nice.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: jsversion-telemetry.patch

Add telemetry to record JSVersion enum values [1]. HTML and XUL content are tracked separately. HTML script defaults [2] to JSVERSION_DEFAULT (JS 1.5) while XUL script defaults [3] to JSVERSION_LATEST (i.e. JSVERSION_ECMA_5).

This patch records both explicit and implicit <script> tag versions to give us a sense of scale, though the number of implicit versions might be uselessly huge. Would it be useful to differentiate between explicit and implicit versions, even when they map to the same JSVersion enum value?

[1] https://hg.mozilla.org/mozilla-central/file/d8b2e3738785/js/src/jspubtd.h#l51
[2] https://hg.mozilla.org/mozilla-central/file/d8b2e3738785/content/base/src/nsScriptLoader.cpp#l544
[3] https://hg.mozilla.org/mozilla-central/file/d8b2e3738785/content/xul/document/src/nsXULContentSink.cpp#l873

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8402834 [details] [diff] [review]
jsversion-telemetry.patch

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

::: toolkit/components/telemetry/Histograms.json
@@ +310,5 @@
>    },
> +  "JS_VERSION": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 7,

Just FYI: if you ever plan to add new items for the values of this histogram, you should make n_values somewhat larger than it is today.  Otherwise, when you add new values, you also need to change your histogram definition.

Doubling to 14 or even just making it an even 20 should provide you with plenty of room to grow, I'd think.

@@ +316,5 @@
> +  },
> +  "JS_VERSION_XUL": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 7,

Ditto here.

Comment 3

[Chris Peterson [:cpeterson]]

Nathan: do my histogram's enumerated values need to be contiguous and zero-based? My patch is collecting JSVersion enum values, which are non-contiguous:

    enum JSVersion {
        JSVERSION_ECMA_3  = 148,
        JSVERSION_1_6     = 160,
        JSVERSION_1_7     = 170,
        JSVERSION_1_8     = 180,
        JSVERSION_ECMA_5  = 185,
        JSVERSION_DEFAULT = 0,
        JSVERSION_UNKNOWN = -1,
        JSVERSION_LATEST  = JSVERSION_ECMA_5
    };

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson (:cpeterson) from comment #3)
> Nathan: do my histogram's enumerated values need to be contiguous and
> zero-based? My patch is collecting JSVersion enum values, which are
> non-contiguous:

Yes, they do.  You'll have to map them into something contiguous, or your histogram numbers will be very misleading.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 8402834 [details] [diff] [review]
jsversion-telemetry.patch

I need to rework the histogram values.

Comment 6

[Till Schneidereit [:till]]

I looked into this some, and realized that we don't need to check for all these versions.

Effectively, there are three version-gated features: `for each`, old-style generators, and `for (var [K, V] in EXPR)`. We only need to check for those versions that enable any of these.

Specifically, those are:

>= 1.6:
- for each

1.7 - 1.8:
- old-style generators

1.7:
- for (var [K, V] in EXPR)

So we only need to test for versions 1.6 to 1.8.5.

I don't think it's necessary to send any data at all if another version is set or the default version is left intact. Also, we don't need to choose a number of histogram entries that'd allow us to add items later on: we're very much committed to not introducing new versions, after all.

(As an aside, these results probably mean that `for (var [K, V] in EXPR)` can be removed at some point, whereas the other two will be hard to remove.)

Comment 7

[Masatoshi Kimura [:emk]]

Don't forget |let|. Authors will have to opt-in to use |let| because we don't support ES6 |let| semantics yet.

Comment 8

[Till Schneidereit [:till]]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> Don't forget |let|. Authors will have to opt-in to use |let| because we
> don't support ES6 |let| semantics yet.

`let` was introduced in JS 1.7, so the probe setup suggested in comment 6 would catch it, too. And you're right: even though we don't want to remove `let`, we do want to make incompatible changes to it, so having some rough data for it from this probe is a good first step.

I wonder if it'd be possible to at least heuristically detect situations where `let` is used in a way that'll cause the changed semantics to have a visible effect. At the very least, I always assumed this probe would be a quick(ish) first step, and we'd do more involved probes later that rely on the parser to inform us about actual usage of the version opt-in-enabled features.

Comment 9

[Chris Peterson [:cpeterson]]

I have patch to add telemetry for JS versions 1.6–1.8. But I'm no longer sure version telemetry will be very useful because we don't know what (if any) version-gated features are used. Plus, other SpiderMonkey language extensions (like shorthand function syntax; see bug 995610) are not version-gated.

Comment 10

[Till Schneidereit [:till]]

(In reply to Chris Peterson (:cpeterson) from comment #9)
> I have patch to add telemetry for JS versions 1.6–1.8. But I'm no longer
> sure version telemetry will be very useful because we don't know what (if
> any) version-gated features are used. Plus, other SpiderMonkey language
> extensions (like shorthand function syntax; see bug 995610) are not
> version-gated.

I really just suggested adding this probe as a very quick first approximation, with more detailed telemetry coming later. The theory was that it should be quite a bit easier to add this probe than to instrument the parser for telemetry. If that's not the case: great. If it is, it would be great to land this probe this week, so we get telemetry for the next cycle. (Maybe even with an uplift to Aurora, so we have Beta in the mix right off the start, but I don't know how likely that is to be accepted.)

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: 988386_jsversion-telemetry-v2.patch

Patch v2 only reports JS minor versions of SpiderMonkey's nonstandard versions (1.6–1.8), so this patch reports counts for just three enumerated values: 6, 7, and 8.

Firefox chrome and addons use lots of nonstandard JS versions, including about:telemetry itself which made testing this patch interesting. :)

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 8421128 [details] [diff] [review]
988386_jsversion-telemetry-v2.patch

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

Nice, thanks for doing this!

I have since realized that we're already sending telemetry gathered within the JS engine for GC-related things, so I think adding more in-depth info about actually used features shouldn't be too hard, too. But this still is a great first step.

(giving r+ instead of f+ because I think this should be able to land as-is.)

::: toolkit/components/telemetry/Histograms.json
@@ +307,5 @@
>      "expires_in_version": "never",
>      "kind": "flag",
>      "description": "Has seen location error"
>    },
> +  "JS_VERSION": {

Nit: perhaps "JS_MINOR_VERSION"?

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8421128 [details] [diff] [review]
988386_jsversion-telemetry-v2.patch

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

This looks great.

::: toolkit/components/telemetry/Histograms.json
@@ +310,5 @@
>    },
> +  "JS_VERSION": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 10,

You may want to make this slightly larger just to allow for future versions.  Your call.

Comment 14

[Till Schneidereit [:till]]

(In reply to Nathan Froyd (:froydnj) from comment #13)
> You may want to make this slightly larger just to allow for future versions.
> Your call.

There won't be any future versions: JS isn't versioned anymore, and there's absolutely no intent to ever change that. The spec is versioned, of course, but there isn't a way for authors to opt in to get newer features: that just happens as they're implemented.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Till Schneidereit [:till] from comment #14)
> (In reply to Nathan Froyd (:froydnj) from comment #13)
> > You may want to make this slightly larger just to allow for future versions.
> > Your call.
> 
> There won't be any future versions: JS isn't versioned anymore, and there's
> absolutely no intent to ever change that. The spec is versioned, of course,
> but there isn't a way for authors to opt in to get newer features: that just
> happens as they're implemented.

Ah, very good, then.  This is why domain expert review overrides my own. :)

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 8421128 [details] [diff] [review]
988386_jsversion-telemetry-v2.patch

Asking a DOM peer for review since these JS telemetry changes touch content/base/* code.

Comment 17

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14a82a4878a8

* Landed with s/JS_VERSION/JS_MINOR_VERSION/
* Telemetry results should show up at http://telemetry.mozilla.org/#nightly/32/JS_MINOR_VERSION

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/14a82a4878a8