Integrate UseCounters in the JS engine

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8898798 [details] [diff] [review]
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)
Attachment #8898798 - Flags: review?(nfroyd)
Attachment #8898798 - Flags: review?(jcoppeard)
(Assignee)

Comment 2

2 years ago
Created attachment 8898799 [details] [diff] [review]
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...
Attachment #8898799 - Flags: review?(nfroyd)
(Assignee)

Comment 3

2 years ago
Created attachment 8898801 [details] [diff] [review]
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.
Attachment #8898801 - Flags: review?(luke)
(Assignee)

Comment 4

2 years ago
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...
Attachment #8898798 - Flags: review?(jcoppeard) → review+
Comment on attachment 8898801 [details] [diff] [review]
3.remove-aot-usage-telemetry.patch

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

Thanks!
Attachment #8898801 - Flags: review?(luke) → review+
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?
Attachment #8898798 - Flags: review?(nfroyd) → review+
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!
Attachment #8898799 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 8

2 years ago
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
Attachment #8898798 - Flags: feedback?(rweiss)
(Assignee)

Comment 9

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

Trying other data peers.
Attachment #8898798 - Flags: feedback?(liuche)
Attachment #8898798 - Flags: feedback?(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+
Attachment #8898798 - Flags: feedback?(rweiss)
Attachment #8898798 - Flags: feedback?(liuche)
Attachment #8898798 - Flags: feedback?(francois)
Attachment #8898798 - Flags: feedback+

Comment 11

2 years ago
Pushed by bbouvier@mozilla.com:
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
(Assignee)

Comment 12

2 years ago
Thanks! Pushing first two changesets, the last one will be pushed later only when we're sure the use counters actually work.
Keywords: leave-open
Priority: -- → P3
(Assignee)

Comment 14

a year ago
Now that bug 1409865 has been demystified, is it a good time to remove the old (non-use counter) telemetry probe?
Flags: needinfo?(luke)
Yes, I think so.
Flags: needinfo?(luke)
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 16

a year ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c75d921ae0
Remove the JS_AOT_USAGE telemetry value; r=luke

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b9c75d921ae0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
status-firefox57: affected → wontfix
You need to log in before you can comment on or make changes to this bug.