Closed Bug 988386 Opened 9 years ago Closed 9 years ago

Add telemetry for JS 1.5 - 1.8 version opt-in

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed

People

(Reporter: till, Assigned: cpeterson)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch jsversion-telemetry.patch (obsolete) — Splinter Review
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
Assignee: general → cpeterson
Status: NEW → ASSIGNED
Attachment #8402834 - Flags: feedback?(till)
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.
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
    };
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
Comment on attachment 8402834 [details] [diff] [review]
jsversion-telemetry.patch

I need to rework the histogram values.
Attachment #8402834 - Flags: feedback?(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.)
Blocks: 1JS
Blocks: 824289
Depends on: 995610
Don't forget |let|. Authors will have to opt-in to use |let| because we don't support ES6 |let| semantics yet.
(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.
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.
(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.)
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. :)
Attachment #8402834 - Attachment is obsolete: true
Attachment #8421128 - Flags: feedback?(till)
Attachment #8421128 - Flags: feedback?(nfroyd)
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"?
Attachment #8421128 - Flags: feedback?(till) → review+
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.
Attachment #8421128 - Flags: feedback?(nfroyd) → feedback+
(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.
(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 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.
Attachment #8421128 - Flags: review?(mrbkap)
Attachment #8421128 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/14a82a4878a8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1046006
Blocks: 867612
No longer blocks: 824289, 1JS
No longer depends on: 995610
Blocks: 1054630
You need to log in before you can comment on or make changes to this bug.