1391633 - Integrate UseCounters in the JS engine

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/use-counters.html

Use counters are a useful way to count the presence of a feature on a web page as well as on a document (the former includes frames, the latter would split them into two different uses). It's basically an overload of a boolean telemetry histogram, but in particular they can be counted per page load and per document load.

CC'ing a few people probably interested in telemetry.
For GC usage, I think we're more interested in values (timings), so this might not be as interesting.
But for deprecated features we want to remove, maybe we could use this to track their usage more precisely and let us know if it's only a few websites mainly using those deprecated features.

Recently, support for custom use counters has landed in bug 1175033. It's easy to cargo-cult the JS telemetry framework to extend it to support use counters as well.

We're interested in doing this for asm.js/wasm support: having the raw count of asm.js/wasm instantiations is merely interesting; we just want to know if the features are used at all.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.usecounter-in-js.patch

Jon for the JS parts, Nathan for the rest and good usage of use counters.

(Cargo-culting worked as expected)

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.remove-unused-cx.patch

Dead JSContext argument in SetDocumentAndPageUseCounter. It might just be a convention in Gecko that a JSContext is always passed, but since I was around...

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.remove-aot-usage-telemetry.patch

Assuming the first patch gets approved and checked in and we get results, we could then remove the histogram value telemetry probe.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Is there a way to nicely test this? I've built a browser with the three patches above and it didn't crash when loading an asm.js real-world module. There was no obvious errors/warnings in the stderr output that related to the particular locations I've touched. But that doesn't tell if telemetry did actually work...

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8898801 [details] [diff] [review]
3.remove-aot-usage-telemetry.patch

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

Thanks!

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8898798 [details] [diff] [review]
1.usecounter-in-js.patch

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

::: dom/base/UseCounters.conf
@@ +92,5 @@
>  attribute DataTransfer.mozSourceNode
> +
> +// JavaScript feature usage
> +custom JS_asmjs uses asm.js
> +custom JS_wasm uses WebAssembly

Since these are adding telemetry probes under the hood, adding these requires data review...I think?

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8898799 [details] [diff] [review]
2.remove-unused-cx.patch

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

That's kind of weird, but removing unused code is a good thing!

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8898798 [details] [diff] [review]
1.usecounter-in-js.patch

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

Good idea, Nathan!

Rebecca: this is adding use counters to measure the amount of pages/documents that have at least one asm.js module or a webassembly module instantiated (thus probably used). It would thus replace the simple telemetry probe called JS_AOT_USAGE and instead use two telementry use counters (one for asm.js, one for wasm). Per the documentation of UseCounters [1], these would never expire. Please let me know if you need more information, thanks.

[1] http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/use-counters.html

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8898798 [details] [diff] [review]
1.usecounter-in-js.patch

Trying other data peers.

Comment 10

[François Marier [:francois]]

Comment on attachment 8898798 [details] [diff] [review]
1.usecounter-in-js.patch

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

This is Category 1 data (https://wiki.mozilla.org/Firefox/Data_Collection#Data_Collection_Categories).

datareview+

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7288fbce6f16
Integrate use counters in the JavaScript engine; r=jonco, r=froydnj, data-review=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4c3bb2c46d
Remove the unused JSContext parameter in SetDocumentAndPageUseCounter; r=froydnj

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks! Pushing first two changesets, the last one will be pushed later only when we're sure the use counters actually work.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7288fbce6f16
https://hg.mozilla.org/mozilla-central/rev/2e4c3bb2c46d

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Now that bug 1409865 has been demystified, is it a good time to remove the old (non-use counter) telemetry probe?

Comment 15

[Luke Wagner [:luke]]

Yes, I think so.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c75d921ae0
Remove the JS_AOT_USAGE telemetry value; r=luke

Comment 17

[Andreea Pavel [:apavel]]

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