1288778 - Baldr: collect telemetry about usage of asm.js vs WebAssembly

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Just an idea for post-MVP release: if we could track the number of asm.js vs wasm modules per page loads, we could keep track of evolution of software used in the wild and also know when it's time to remove asm.js specific optimizations.

It is apparently quite easy to add a telemetry probe. My search so far led me to:
- https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
- https://wiki.mozilla.org/Telemetry
- JSRuntime::addTelemetry (as of today it's there: http://searchfox.org/mozilla-central/source/js/src/vm/Runtime.cpp#480 )

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: telemetry.patch

This adds the possibility to track usage of AsmJS vs WebAssembly in a telemetry probe.

This will count the number of modules instantiations per user, then aggregates this on our telemetry servers. Counting the number of times a module is *linked* is convenient, because we then have a JSContext which can provide us a JSRuntime (needed for calling the telemetry probe); we're not sure to be on a JSContext whenever we just compile the module; that measure would probably be more representative, though (especially in the future when we may have SAB and a single module compiled once and instantiated N times).

Asking feedback? for the new data collected. The motivation behind it is to know when usage of wasm overcomes usage of asm.js, and then we could reduce asm.js optimizations, if not remove them at all.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8789389 [details] [diff] [review]
telemetry.patch

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

Wow, it's cool to see how little work this is.  As suggested on IRC, Module::instantiate() might be a better plays and then you can have it in one place instead of two.

So will this aggregate the simple raw numbers of each per browser session?  Is there any way to get a "uses per page-load" statistic like we see in https://www.chromestatus.com/metrics/feature/timeline/popularity/473?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: telemetry.v2.patch

Updated patch, carrying forward r+.

Luke: it's not possible to have per-page-load measurements yet; there's bug 1297457 that could allow this in the future (but even then, maybe the ratio would have to be done by hand?). So what we'll have absolute raw usage count instead.

Comment 4

[Benjamin Smedberg]

Comment on attachment 8789728 [details] [diff] [review]
telemetry.v2.patch

1) Please improve the description. It should describe *what* is being recorded and *when*. It's not clear from reading this description what would trigger this being recorded. What values can be in the "key" for this keyed histogram? I don't understand why there's a key at all here. In fact reading the code, I don't think you're recording this with a key, so I'm not sure that this actually works.
2) Do you really need n_value: 10? You currently only have two things in your enumeration: do you expect to add more soon?
3) I don't think this is ready for expires_in_version: never. Right now this seems like exploratory data collection. Let's have this expire in 56 (that's six months), and at that time if the data is valuable and you have a long-term monitoring plan we can discuss collecting forever.
3a) I encourage people to add automated tests for any probe, but forever telemetry or opt-in telemetry really needs automated tests to make sure we don't accidentally break a probe over time.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> Comment on attachment 8789728 [details] [diff] [review]
> telemetry.v2.patch
> 
> 1) Please improve the description. It should describe *what* is being
> recorded and *when*. It's not clear from reading this description what would
> trigger this being recorded. What values can be in the "key" for this keyed
> histogram? I don't understand why there's a key at all here. In fact reading
> the code, I don't think you're recording this with a key, so I'm not sure
> that this actually works.

I've changed the description. Indeed the key might not be needed; sorry for the bad copy/pasto here.

> 2) Do you really need n_value: 10? You currently only have two things in
> your enumeration: do you expect to add more soon?

Initial plan was to record the version number of WebAssembly, but we are very likely to maintain only one at a time anyway, so I've reverted to just include asm.js vs WebAssembly. Only 2 are needed at the moment; the wiki page stated to keep some ballast, just in case this would change in the future. I don't see any compelling reason to have more right now (but I wouldn't have predicted WebAssembly two years ago either). If that can be changed easily in the future, probably keeping it to 2 right now sounds good.

> 3) I don't think this is ready for expires_in_version: never. Right now this
> seems like exploratory data collection. Let's have this expire in 56 (that's
> six months), and at that time if the data is valuable and you have a
> long-term monitoring plan we can discuss collecting forever.
> 3a) I encourage people to add automated tests for any probe, but forever
> telemetry or opt-in telemetry really needs automated tests to make sure we
> don't accidentally break a probe over time.

Sounds good to have this expire in 56. The initial, stable version of WebAssembly should be officially released around January, for what it's worth, but we can change this in the future easily, I assume.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: telemetry.v3.patch

Carrying forward r+ from luke.

Comment 7

[Benjamin Smedberg]

Comment on attachment 8790188 [details] [diff] [review]
telemetry.v3.patch

you can't add buckets later without renaming. So for now I'd go with 4 to leave a little wiggle. data-review=me

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/140e1df48538
Add a telemetry probe for usage of asm.js / wasm; r=luke, data-review=bsmedberg

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/140e1df48538