Run telemetry to see whether we can drop explicit support for day of week late in the date format
Categories
(Core :: JavaScript: Standard Library, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox121 | --- | fixed |
People
(Reporter: vinny.diehl, Assigned: vinny.diehl)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
2.96 KB,
text/plain
|
chutten
:
data-review+
|
Details |
While working on day of week support in bug 1617562, I discovered that we accept day of week in places which other engines reject:
Format | JSC | SM | V8 |
---|---|---|---|
"Sep 26 1995 Tues" |
❌ | ✔️ | ❌ |
"Sep 26 Tues 1995" |
❌ | ✔️ | ❌ |
"Sep 26 1995 10:Tues:00" |
❌ | ✔️ | ❌ |
See the aforementioned bug and its patch for more details about where day of week should be accepted.
The purpose of this bug is to collect data on whether removing support for these oddball patterns would break anything. If my calculations are correct, after D192560 lands, action
should never == 0
except in the oddball cases mentioned, so we'll wait for that to land.
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
Obviously I'll need to pass the JSContext
into ParseDate
, but then what should I use as the JSObject
required by JSRuntime::setUseCounter
?
Comment 2•2 years ago
|
||
Thank you for looking into this!
Here's a document for the use counter.
Sorry I should've pointed this document first.
https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/collection/use-counters.html
Inside SpiderMonkey, the use counter is implemented as a callback to the embedding (Gecko).
void JSRuntime::setUseCounter(JSObject* obj, JSUseCounter counter) {
if (useCounterCallback) {
(*useCounterCallback)(obj, counter);
}
}
void JSRuntime::setUseCounterCallback(JSRuntime* rt,
JSSetUseCounterCallback callback) {
rt->useCounterCallback = callback;
}
The callback is set by JS_SetSetUseCounterCallback
,
void JS_SetSetUseCounterCallback(JSContext* cx,
JSSetUseCounterCallback callback) {
cx->runtime()->setUseCounterCallback(cx->runtime(), callback);
and it's set here, with SetUseCounterCallback
.
JS_SetSetUseCounterCallback(cx, SetUseCounterCallback);
the SetUseCounterCallback
converts the JS-specific use couter type (JSUseCounter
) to the global use counter type (UseCounter
).
static void SetUseCounterCallback(JSObject* obj, JSUseCounter counter) {
switch (counter) {
case JSUseCounter::ASMJS:
SetUseCounter(obj, eUseCounter_custom_JS_asmjs);
break;
case JSUseCounter::WASM:
SetUseCounter(obj, eUseCounter_custom_JS_wasm);
break;
The passed JSObject*
is used for retrieving the document associated for the counter
(so, the counter value means "this feature is used in this document").
The object can be any object that belongs to the document's global.
In the Date
's case, it can for example be Date
constructor or the Date
prototype, or just the global object cx->global()
.
void SetUseCounter(JSObject* aObject, UseCounter aUseCounter) {
nsGlobalWindowInner* win =
xpc::WindowGlobalOrNull(js::UncheckedUnwrap(aObject));
if (win && win->GetDocument()) {
win->GetDocument()->SetUseCounter(aUseCounter);
The existing consumer in wasm uses the wasm instance object as the JSObject*
parameter.
instance.set(WasmInstanceObject::create(
...
std::move(maybeDebug)));
...
JSUseCounter useCounter =
metadata().isAsmJS() ? JSUseCounter::ASMJS : JSUseCounter::WASM;
cx->runtime()->setUseCounter(instance, useCounter);
Then, the use counter type is defined in dom/base/UseCounters.conf
.
SpiderMonkey's use counter uses custom
type (see the document above).
// JavaScript feature usage
custom JS_asmjs uses asm.js
custom JS_wasm uses WebAssembly
and that's processed by gen-usecounters.py
script:
GeneratedFile(
"UseCounterList.h",
script="gen-usecounters.py",
entry_point="use_counter_list",
inputs=["UseCounters.conf"],
The JS-specific use counter type is defined in js/public/friend/UsageStatistics.h
,
and it's converted in SetUseCounterCallback
above.
enum class JSUseCounter { ASMJS, WASM };
let me know if there's anything unclear.
Assignee | ||
Comment 3•2 years ago
|
||
Thank you for the incredibly detailed breakdown!
My plan is to set a use counter for every successfully parsed implementation-specific Date format (so, ParseISOStyleDate
and rejected formats will not be counted), and another counter when one of the offending formats are encountered, so that we may calculate what percentage of implementation-specific dates encountered require support for such formats.
I'd like to make unit tests to confirm that the offending formats are indeed sending telemetry, and that the normal formats parsed by the bug 1617562 code are not sending telemetry. I found this in the documentation, but an example of how to use these functions would be helpful.
Assignee | ||
Comment 4•2 years ago
|
||
Updated•2 years ago
|
Assignee | ||
Comment 5•2 years ago
|
||
The patch I've just uploaded has been manually tested using about:telemetry
, but if there's an easier/automated way, it'd be good to know for next time (and, I still will add the tests, just to be sure).
Assignee | ||
Comment 6•2 years ago
|
||
Assignee | ||
Updated•2 years ago
|
Comment 7•2 years ago
|
||
(In reply to Vinny Diehl from comment #3)
I'd like to make unit tests to confirm that the offending formats are indeed sending telemetry, and that the normal formats parsed by the bug 1617562 code are not sending telemetry. I found this in the documentation, but an example of how to use these functions would be helpful.
:chutten, how is the use counter supposed to be tested?
I've looked into the history of UseCounters.conf, but couldn't locate a patch with tests.
Assignee | ||
Comment 8•2 years ago
|
||
It looks like I should be able to pull the histogram with Services.telemetry.getHistogramById("USE_COUNTER2_JS_LATE_WEEKDAY_DOCUMENT")
, but I'm having trouble getting Date.parse
to run in a way that affects the telemetry. I'm running as an xpcshell-test like this one:
https://searchfox.org/mozilla-central/source/js/xpconnect/tests/unit/test_js_memory_telemetry.js
However, simply executing new Date(...)
directly in the add_task
callback doesn't affect the telemetry, even after a few seconds delay. I also tried to eval the Date parse in Cu.evalInSandbox
, but still nothing. I've also looked at the data from Services.telemetry.getSnapshotForHistograms("main", true).parent
and can't find the histogram there either.
Any ideas what I'm doing wrong arai? Is there some particular way that I need to run the Date.parse
function, or am I using the wrong type of test? If not, perhaps I'm going about pulling the telemetry data incorrectly.
Comment 9•2 years ago
|
||
good find :)
My guess is that, xpcshell doesn't work well with the use counter, given the use counter's data is associated with document, while the code executed in xpcshell isn't necessarily associated with document.
void SetUseCounter(JSObject* aObject, UseCounter aUseCounter) {
nsGlobalWindowInner* win =
xpc::WindowGlobalOrNull(js::UncheckedUnwrap(aObject));
if (win && win->GetDocument()) {
win->GetDocument()->SetUseCounter(aUseCounter);
You could use "mochitest browser" instead.
example tests in https://searchfox.org/mozilla-central/source/js/xpconnect/tests/browser
Tests for mochitest browser is named browser_*.js
in the source. e.g.: browser_dead_object.js,
and the file is listed in browser.toml
file in the same directory. e.g.: browser.toml
The function passed to add_task
is executed with the browser's scope, so I suppose the telemetry doesn't work here as well.
You need to open a tab and let the page opened in the tab execute the new Date(...)
, so that the result is associated with the document, and eventually reflected to the telemetry result.
let newTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, url);
the test can be run by ./mach test PATH_TO_TEST
after ./mach build
for browser.
Assignee | ||
Comment 10•2 years ago
|
||
Good point! Alas, I still seem to be screwing up somewhere. Here is the code I have so far, just to try and figure out how to get the telemetry to register:
const HIST_ID = "USE_COUNTER2_JS_LATE_WEEKDAY_DOCUMENT";
// This URL sets telemetry if I try it manually in a browser via `mach run`
const URL = "data:text/html;charset=utf-8,<script>new Date('Sep 26 1995 Tues')</script>";
add_task(async function test_date_telemetry() {
console.log("***************************************************\n",
"Before:", Services.telemetry.getHistogramById(HIST_ID)); //=> {}
let newTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, URL);
// ghetto sleep for now, will eventually use a proper timeout loop if necessary
let d = Date.now();
while (Date.now() - d < 5000);
console.log("***************************************************\n",
"After:", Services.telemetry.getHistogramById(HIST_ID)); //=> {}
});
I have also tried saving the code to a proper .html
file and opening the page in the mochitest that way, still nothing. I've also grepped the full list of histograms from getSnapshotForHistograms
and there's still nothing in there.
Comment 11•2 years ago
|
||
I think you need to close the tab before getting the telemetry result.
SetUseCounter
above calls Document::SetUseCounter
,
void SetUseCounter(JSObject* aObject, UseCounter aUseCounter) {
...
win->GetDocument()->SetUseCounter(aUseCounter);
and Document::SetUseCounter
stores the result into Document::mUseCounters
void SetUseCounter(UseCounter aUseCounter) {
mUseCounters[aUseCounter] = true;
the Document::mUseCounters
is used in the following
void Document::ReportDocumentUseCounters() {
...
// Report our per-document use counters.
for (int32_t c = 0; c < eUseCounter_Count; ++c) {
auto uc = static_cast<UseCounter>(c);
if (!mUseCounters[uc]) {
...
Telemetry::Accumulate(id, 1);
and Document::ReportDocumentUseCounters
is called in Document::Destroy
.
void Document::Destroy() {
...
ReportDocumentUseCounters();
so, try closing the tab with BrowserTestUtils.removeTab(tab);
and then get the telemetry data
Assignee | ||
Comment 12•2 years ago
•
|
||
Man, you are fast with the detailed info! Unfortunately, that didn't work :(
When manually testing, the tab isn't required to be closed before the result shows up in about:telemetry
, which is funny, because ReportDocumentUseCounters
doesn't appear to be called anywhere but the destructor and nsXMLContentSink::OnTransformDone
... perhaps this is being periodically updated elsewhere? Regardless, this may help to process the telemetry more quickly so that a timeout won't be necessary once we do figure this out, so I'll keep this in mind.
Any more ideas?
Comment 13•2 years ago
|
||
if the behavior can be observed in manual test, the thing to check is that whether it also works with mochitest's environment.
You can put await new Promise(() => {});
inside the test to pause it (the test waits for never-resolving promise there).
With the change, run the test, and once it pauses, manually operate on the browser window in the same way as manual test, and check about:telemetry
.
(you'll need to pass --timeout=60000
or so to ./mach test ...
, to avoid hitting timeout during manual test)
If you can observe the same behavior there, then the issue would be somewhere between the Services.telemetry.getHistogramById
vs the about:telemetry
's data source
If you cannot observe the same behavior there, then the issue would be something in the mochitest's environment
(possibility is that you need some configuration to enable telemetry there)
Assignee | ||
Comment 14•2 years ago
|
||
Nice tip on pausing the test, thanks!
The histograms for USE_COUNTER2_JS_LATE_WEEKDAY_DOCUMENT
and USE_COUNTER2_JS_LATE_WEEKDAY_PAGE
are both present in about:telemetry
during the mochitest, so we've at least landed on the right way to trigger the telemetry during testing.
I'll wait to see if :chutten has any more insight on how to retrieve that data. :)
Comment 15•2 years ago
|
||
There are a couple of things likely going awry here:
Telemetry.getHistogramById(...)
returns a JSHistogram object which has methods on it. One of those methods issnapshot()
which would give you what you needed if your histogram's data is on theparent
process. If the use counter is on thecontent
process, you get to usegetSnapshotForHistograms
, and learn about IPC in point (2).- Telemetry IPC operates every couple of seconds. To wait for the data to arrive, the current art is to use
waitForCondition
and have it wait until the provided value matches the expected value.
You might find it easier to integrate your use counter test with an existing one like the ones in https://searchfox.org/mozilla-central/source/dom/base/test/browser_use_counters.js -- it has it all sorted out and ought to make your life easier. (And mine, as I'm adding Glean versions of use counters in bug 1852098, so I'll be adding Glean tests while I'm in the neighbourhood.). Another option is to wait for bug 1852098 to land, and then test the Glean version instead with its much nicer test APIs... but I think adding it to browser_use_counters.js
is probably best.
Assignee | ||
Comment 16•2 years ago
|
||
One of those methods is snapshot() which would give you what you needed if your histogram's data is on the parent process
Yesssss, thank you! The {}
in the console.log output was throwing me off, I thought it just wasn't finding any histogram at all. In light of this information, I think a bespoke test is the easiest option, given what I'm trying to do. I have it working now, I'll have a patch up in a minute.
Comment 17•2 years ago
|
||
Comment on attachment 9363144 [details]
Request for data collection form
PRELIMINARY NOTES:
Use Counters have no in-built expiry mechanism, so I'm going to review this as though it will actually be a permanent collection.
Note that, after bug 1862546, most new use counters will be covered by its data review and not need individual reviews thereafter.
DATA COLLECTION REVIEW RESPONSE:
Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?
Yes.
Is there a control mechanism that allows the user to turn the data collection on and off?
Yes. This collection can be controlled through Firefox's Preferences.
If the request is for permanent data collection, is there someone who will monitor the data over time?
Yes, Vinny Diehl is responsible.
Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Category 1, Technical.
Is the data collection request for default-on or default-off?
Default on for all channels.
Does the instrumentation include the addition of any new identifiers?
No.
Is the data collection covered by the existing Firefox privacy notice?
Yes.
Does the data collection use a third-party collection tool?
No.
Result: datareview+
Comment 18•2 years ago
|
||
Comment 19•2 years ago
|
||
bugherder |
Comment 20•2 years ago
|
||
bugherder |
Description
•