Closed Bug 1322292 Opened 3 years ago Closed 3 years ago

Performance.now() is broken on workers

Categories

(Core :: DOM: Core & HTML, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached patch part 1Splinter Review
We are not following what the spec says. The first patch moves Now() to the Performance.cpp.
The second patch uses creationTime instead of nowBase in workers.
Attachment #8817063 - Flags: review?(bugs)
Attached patch part 2 (obsolete) — Splinter Review
Attachment #8817065 - Flags: review?(bugs)
Well, I don't think the Performance spec acknowledged the existence of workers when we first implemented this.  Still, good to conform if there is a spec now.

Are their wpt tests?
I think bz should review this, since, IIRC, he was dealing with .now() issues in the spec.
Attachment #8817063 - Flags: review?(bugs)
Attachment #8817065 - Flags: review?(bugs)
Bz, can you please take a look at these 2 patches?
Flags: needinfo?(bzbarsky)
Comment on attachment 8817063 [details] [diff] [review]
part 1

>+  virtual DOMHighResTimeStamp Now() const;

Doesn't need to be virtual anymore, right?

r=me
Flags: needinfo?(bzbarsky)
Attachment #8817063 - Flags: review+
Comment on attachment 8817065 [details] [diff] [review]
part 2

It's not clear to me that the console behavior is right.  Why is it right?  That is, where do these TimeDurations actually get used?  Note that a page and its workers share a console, and showing timestamps with different 0 times in there is a bit confusing.  But that's already a problem to some extent for subframes, I guess?

The comment in Event::TimeStamp no longer matches its implementation.  Which one matches the spec?  r- for this part at the very least, because _something_ in here needs changing.

It's not clear to me that the PerformanceWorker::GetPerformanceTimingFromString change is correct either.  What does the spec say there?
Attachment #8817065 - Flags: review-
> That is, where do these TimeDurations actually get used?

It's used for: console.time('foo'); something(); console.timeEnd('foo'); and you see how long it took to run something().

> and its workers share a console, and showing timestamps with different 0

This is not actually true. console is not shared between workers and windows (or iframes). Each context has its own console object and these time/timeEnd are per context.

> The comment in Event::TimeStamp no longer matches its implementation.  Which
> one matches the spec?  r- for this part at the very least, because
> _something_ in here needs changing.

The spec talks about returning the epoch time (from 1970/01/01). And this is what happens in release. But in our nightly we return Now() - navigationStart (on main-thread). It seems correct to me to return now() - creationTime on workers.
Note that creationTime and nowBaseTime are equal only for the top-level worker. NowBaseTime value for a subWorker is equal to the creationTime of the parent worker.

> It's not clear to me that the
> PerformanceWorker::GetPerformanceTimingFromString change is correct either. 
> What does the spec say there?

For workers navigationStart should be equal to fetchStart, and that is when the WorkerPrivate is created (in theory when the fetch operation starts, but it's almost the same value of the WorkerPrivate creation), so yes, this seems correct. Definitely better than what we had before: we return the value of the creationTime of the parent worker.
Flags: needinfo?(bzbarsky)
> It's used for: console.time('foo'); something(); console.timeEnd('foo');

So is the actual value in "monotonicTimer" never exposed?  Only differences between two values?  That is, it doesn't matter what timestamp we actually subtract as long as we're consistent?  Or is the actual value exposed somewhere?

> console is not shared between workers and windows (or iframes).

I mean the actual console the web developer observes, not the Console object.  Again, if we never show the "monotonicTimer" values in there this doesn't matter.

> The spec talks about returning the epoch time (from 1970/01/01).

Sort of.  It then has a "this part of the spec is a lie, see the issue" box pointing at <https://github.com/whatwg/dom/issues/23>.

I guess following the links from there lands us at "use the same time base as performance.now()" in <https://lists.w3.org/Archives/Public/public-whatwg-archive/2014May/0046.html>, so the code change is correct...  The comment needs fixing.

I wonder whether we can just call through to some method on the worker's Performance object to avoid the code duplication here and centralize the concept of zero time for the worker a bit more...  Maybe worth a followup.

> Note that creationTime and nowBaseTime are equal only for the top-level worker.

I'm not sure what this is trying to say.  Is this a comment on today's code?  In today's code for a top-level dedicated worker nowBaseTime is the same as for its parent page, which is generally different from the creationTime of the worker.

> For workers navigationStart should be equal to fetchStart

Why?  What spec says this and where?  I tried sorting through the rat's nest of specs involved but they have tons of broken links and I can't find anything actually defining this.
Flags: needinfo?(bzbarsky)
> What spec says this and where?

Fwiw, I think the answer is "none; there is absolutely no spec that actually defines the behavior of this API in workers in any implementable way".  See also https://lists.w3.org/Archives/Public/public-web-perf/2016Dec/0007.html
> So is the actual value in "monotonicTimer" never exposed?  Only differences
> between two values?  That is, it doesn't matter what timestamp we actually
> subtract as long as we're consistent?  Or is the actual value exposed
> somewhere?

In theory, it's exposed to devtools. But they don't use it. They just use the duration attribute from console.timeEnd('foo').
I'll file a follow up for get rid of 'started' in ConsoleTimerStart.

> > For workers navigationStart should be equal to fetchStart

Yeah, only in w3.org/TR/navigation-timing there is that definition.

https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart
"This attribute must return the time immediately after the user agent finishes prompting to unload the previous document. If there is no previous document, this attribute must return the same value as fetchStart."
Attached patch part 2Splinter Review
Attachment #8817065 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attached patch part 3Splinter Review
WorkerPrivate::TimeStampToDOMHighRes()
> Yeah, only in w3.org/TR/navigation-timing there is that definition.

That spec is explicitly not applicable to workers.
Comment on attachment 8818841 [details] [diff] [review]
part 2

>+++ b/dom/performance/PerformanceWorker.cpp
> PerformanceWorker::GetPerformanceTimingFromString(const nsAString& aProperty)
>   if (aProperty.EqualsLiteral("navigationStart")) {

OK.  So per the spec discussion starting at https://lists.w3.org/Archives/Public/public-web-perf/2016Dec/0007.html this is all wrong.  IsPerformanceTimingAttribute() should just return null for workers, and we shouldn't have this code be reachable at all, presumably.  Please file a followup bug on this, though we should check what other UAs do.

r=me with that bug and the console bug filed.
Flags: needinfo?(bzbarsky)
Attachment #8818841 - Flags: review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/291a68ca4825
Some fixes for the Performance API in workers - part 1 - Centralized Performance.now(), r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/301231f4165a
Some fixes for the Performance API in workers - part 2 - Get rid of NowBaseTimeStamp(), r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/b827e4d0dc73
Some fixes for the Performance API in workers - part 3 - TimeStampToDOMHighRes() in workerPrivate, r=bz
Blocks: 1323676
Depends on: 1323941
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.