908390 - [Workers] Implement Performance.now() for workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Since we have PerformanceTiming.now() for high-precision timing on the main thread, it would be nice to have the same feature for workers.

Investigation is in progress but I suspect that discrepancies in Date.now() on a worker are causing bug 874425.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

PerformanceTiming is something else entirely (it's performance.timing).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ms2ger, are you interested in doing this?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I probably can. Dunno when I'll have time, though.

Comment 4

[:Irving Reid (No longer working on Firefox)]

It would be very useful to have performance.now() for all chrome code, not just workers - telemetry suffers greatly from non-monotonic Date.now(). Should we scope-creep this bug, or file another?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Another.  The change to hang a performance object off a random non-WebIDL mainthread global is quite different from doing it off a worker global.

Comment 6

[Jukka Jylänki]

This would be very useful for measuring performance in SharedArrayBuffer+Mutex work in bug https://bugzilla.mozilla.org/show_bug.cgi?id=979594

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make [Exposed] work correctly on jsonifiers and stringifiers and don't create an unforgeable toJSON just because we have a jsonifier

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Implement performance.now() on workers

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Implement performance.now() on workers

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8476950 [details] [diff] [review]
part 1.  Make [Exposed] work correctly on jsonifiers and stringifiers and don't create an unforgeable toJSON just because we have a jsonifier

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

::: dom/bindings/Codegen.py
@@ +2128,5 @@
>          if not static:
>              stringifier = descriptor.operations['Stringifier']
>              if (stringifier and
> +                unforgeable == MemberIsUnforgeable(stringifier, descriptor) and
> +                isMaybeExposedIn(stringifier, descriptor)):

The continuation should be over on the right (aligned with the other args).

@@ +2143,5 @@
>                      self.regular.append(toStringDesc)
>              jsonifier = descriptor.operations['Jsonifier']
> +            if (jsonifier and
> +                unforgeable == MemberIsUnforgeable(jsonifier, descriptor) and
> +                isMaybeExposedIn(jsonifier, descriptor)):

here too.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8476954 [details] [diff] [review]
part 2.  Implement performance.now() on workers

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

Ideally we would run whatever test we have for performance.now in a window inside a worker too ...

::: dom/workers/WorkerScope.cpp
@@ +55,5 @@
>                                                    DOMEventTargetHelper)
>    tmp->mWorkerPrivate->AssertIsOnWorkerThread();
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mConsole)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPerformance)
> +  // XXXbz what about mLocation and mNavigator?

Uh, excellent point.  File a bug, please?

::: dom/workers/test/test_worker_interfaces.js
@@ +97,5 @@
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "MessagePort",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "Performance",
> +// IMPORTANT: Do not change this list without review from a DOM peer!

I totally missed where we created this test.  It's beautiful.

Comment 12

[Boris Zbarsky [:bzbarsky]]

>The continuation should be over on the right (aligned with the other args).
...
>here too.

Ignored, per irc.

> Ideally we would run whatever test we have for performance.now in a window inside a
> worker too ...

Done.

> Uh, excellent point.  File a bug, please?

Bug 1060621.

> I totally missed where we created this test. 

bkelly rocks.  ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Followup to fix randomorange in test_eventTimeStamp.html

Landed this, but would like a once-over review for it.  Note that this caught a bug: in the WorkerPrivateParent ctor, we need to use mLoadInfo, not aLoadInfo, since we already did mLoadInfo.StealFrom(aLoadInfo)

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f79c41150d2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d549361c6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/42c3c548cdb3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f79c41150d2b
https://hg.mozilla.org/mozilla-central/rev/02d549361c6d
https://hg.mozilla.org/mozilla-central/rev/42c3c548cdb3

Comment 16

[Sylvestre Ledru [:Sylvestre]]

"Performance.now() for workers implemented" => added to the release notes.

Comment 17

[Jean-Yves Perrier [:teoli]]

In order to document this, I have a few assertions to check:
- This bug makes Performance (but only with now() ) available on WorkerGlobalScope — and therefore (Shared|ServiceWorker)GlobalScope —, the global objects of worker
- Performance.now() — introduced in Fx 15 — is the only method/attribute available to both workers and non-worker context. All the other method/attributes are only available on non-worker contexts.

Is there a plan, or a wish, to make the rest of Performance available to workers in the future? If not, why not creating a WorkerPerformance interface (like WorkerNavigator, WorkerLocation, …)

Comment 18

[Boris Zbarsky [:bzbarsky]]

> - This bug makes Performance (but only with now() ) available on WorkerGlobalScope — and
> therefore (Shared|ServiceWorker)GlobalScope —, the global objects of worker

Correct, this is exposed in all worker types.  The way the 0 time is determined is different for different worker types, though.  Specifically:

1)  A shared or service worker uses the worker start time as 0.
2)  A dedicated worker started from a Window uses that window's navigationStart as a
    0 (like performance.now() in a window).
3)  A dedicated worker started from another worker uses that other worker's 0 time as
    its 0 time.

> Performance.now() — introduced in Fx 15 — is the only method/attribute available to both
> workers and non-worker context.

Correct.

> Is there a plan, or a wish, to make the rest of Performance available to workers in the
> future? 

Not so far.

> If not, why not creating a WorkerPerformance interface (like WorkerNavigator,
> WorkerLocation

Because those interfaces were a specification hackaround for the problem of not having [Exposed] in IDL when they were defined.  Now that we have [Exposed] we are no longer adding worker-specific interfaces if we just want to subset functionality.

Comment 19

[Jean-Yves Perrier [:teoli]]

Thank you very much, I have adapted the way we document interfaces to be able to indicate attribute/methods not exposed to workers.

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/Performance
https://developer.mozilla.org/en-US/docs/Web/API/Performance.now
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope.performance
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope
and
https://developer.mozilla.org/en-US/Firefox/Releases/34

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I have adapted the way we document interfaces to be able to indicate attribute/methods
> not exposed to workers

Note that there can also be things exposed to workers but not windows. And things exposed to some kinds of workers but not other kinds..

Comment 21

[Boris Zbarsky [:bzbarsky]]

And thank you for documenting this!

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8481756 [details] [diff] [review]
Followup to fix randomorange in test_eventTimeStamp.html

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

Nice.  Tests ftw.