User Timing API is not available on Workers

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ferjm, Assigned: amarchesini)

Tracking

({dev-doc-complete})

unspecified
mozilla41
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, relnote-b2g ?)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
Component: DOM → DOM: Workers
What interface do you mean?  Performance.timing?  That's not available in workers at all.
I guess you mean this:

  http://www.w3.org/TR/user-timing/

Yea, not on workers at all yet.
Summary: User Timing API is not available on ServiceWorkers → User Timing API is not available on Workers
(Reporter)

Updated

3 years ago
Blocks: 1153312
(Reporter)

Comment 3

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #2)
> I guess you mean this:
> 
>   http://www.w3.org/TR/user-timing/
> 
> Yea, not on workers at all yet.

Exactly.

I was trying to run Raptor [1] tests to measure service workers performance.

[1] https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/Raptor

Comment 4

3 years ago
Correct, User Timing relies on DOM timing APIs, and hence isn't available in service workers. CC-ing qDot in case he has anything else to add, since he wrote the original implementation.
I don't think user-timing is spec'd to be on workers.  We are somewhat non-spec conformant in that we allow Performance.now() on workers.
Yeah, user timing is a DOM API, so workers can't use it. bkelly's suggestion of Performance.now() is probably the best we got for the moment.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Per spec, even performance.now() is supposed to not exist in workers.  But that's because the spec is written by a working group that's ... yeah.  Anyway.

It would make some sense to support user timing in workers if we can figure out how it should work...
Yeah, there seems to be some consensus that we could shuffle times back and forth from a worker since the API is not hitting nodes. Will at least reopen since it's feasible.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Updated

3 years ago
Depends on: 1158848
(Reporter)

Updated

3 years ago
Blocks: 1158848
No longer depends on: 1158848
(Reporter)

Updated

3 years ago
See Also: → bug 1159154

Comment 9

3 years ago
Is the goal here to be able to use Performance.mark() on both the main thread and the service worker so that Performance.measure() can be used to measure the latency from the main thread to the SW and vice versa?
I'll take this.  Hopefully have something landed mid-May.
Assignee: nobody → bkelly
Status: REOPENED → ASSIGNED
Comment 9 is a good question... because service workers and mainthread don't have a common 0 time, so you can't compare marks between them.
I think we will have to do some kind of time normalization when the values are read by a script.
I think whoever wants to implement this should talk to the webperf wg to figure out what the heck their plans are here.  Assuming they have any.
Eli, can you outline how you want this to work for your Raptor framework?

Note that SharedWorkers and ServiceWorkers currently have a zero-time corresponding to worker startup, not the navigation start for the window.
Flags: needinfo?(eperelman)

Comment 15

3 years ago
I've tried to keep Raptor pretty agnostic to the types of tests needing to be written. Redirecting to Fernando as he is writing the test.
Flags: needinfo?(eperelman) → needinfo?(ferjmoreno)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> Is the goal here to be able to use Performance.mark() on both the main
> thread and the service worker so that Performance.measure() can be used to
> measure the latency from the main thread to the SW and vice versa?

I think the goal is to have an independent Performance.mark and Performance.measure for the Worker. Especially since trying to associate Performance.mark from the main thread and the worker is a bit tricky as there could be multiple context using the Service Worker (or a SharedWorker).
(Reporter)

Comment 17

3 years ago
Having Performance.mark and Performance.measure on the worker scope would be a good start indeed.

But it would be great if we could also be able to do cross thread measurements. I am probably missing a lot here, but I was wondering if it would be possible to expose a way to modify the zero time of a scope. Or as an alternative to that, to add an extra argument on Performance.mark indicating an offset between scopes.
Flags: needinfo?(ferjmoreno)

Comment 18

3 years ago
Changing the signature on a standard method for this intent seems like a violation of concerns. Measuring A difference between 2 points not designated to navigationStart seems better suited to performance.measure.
I propose we expose Performance.measure/mark on Worker, but keep it isolated to the worker.  The marks are not shared with marks from the window.  I think this is consistent with how these methods work between a window and an iframe.

We could then add something to get an absolute time reference for the zero-time.

You could then use postMessage() to communicate the measured values and the time offset back to the window.

Thoughts?
(Reporter)

Comment 20

3 years ago
I'm ok with this solution. Thank you!
Ok, I will likely start working on this when I get back from PTO around May 13.
I'm working on this API. bkelly, I'm stealing this bug from you.
Assignee: bkelly → amarchesini
Created attachment 8606434 [details] [diff] [review]
timing.patch

mochitests are missing. I'll add it and then I'll ask for a review.
Created attachment 8606915 [details] [diff] [review]
timing.patch
Attachment #8606434 - Attachment is obsolete: true
Attachment #8606915 - Flags: review?(ehsan)
Created attachment 8606939 [details] [diff] [review]
timing.patch
Attachment #8606915 - Attachment is obsolete: true
Attachment #8606915 - Flags: review?(ehsan)
Attachment #8606939 - Flags: review?(ehsan)
Created attachment 8607468 [details] [diff] [review]
timing.patch
Attachment #8606939 - Attachment is obsolete: true
Attachment #8607222 - Attachment is obsolete: true
Attachment #8606939 - Flags: review?(ehsan)
Attachment #8607468 - Flags: review?(ehsan)
Has there been an intent to implement/ship here?  Has someone actually contacted the public-web-perf working group?

Comment 29

3 years ago
Comment on attachment 8607468 [details] [diff] [review]
timing.patch

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

r- mostly because of the way you're delegating Traverse/Unlink to the base class.

::: dom/base/PerformanceEntry.cpp
@@ +27,4 @@
>    mName(aName),
>    mEntryType(aEntryType)
>  {
> +  // mParent is null in workers.

Can you lease convert this into a MOZ_ASSERT?

::: dom/base/PerformanceMark.cpp
@@ +17,2 @@
>  {
> +  // mParent is null in workers.

Same.

::: dom/base/PerformanceMeasure.cpp
@@ +17,4 @@
>    mStartTime(aStartTime),
>    mDuration(aEndTime - aStartTime)
>  {
> +  // mParent is null in workers.

Ditto.

::: dom/base/nsPerformance.cpp
@@ +401,5 @@
>  
>  
>  NS_IMPL_CYCLE_COLLECTION_CLASS(nsPerformance)
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsPerformance, DOMEventTargetHelper)

Don't you need to change the base class here?

@@ +409,5 @@
>    tmp->mMozMemory = nullptr;
>    mozilla::DropJSObjects(this);
> +
> +  // Unlink method from the PerformanceBase class.
> +  tmp->Unlink();

Why not make this work the same was as all other cycle collectible classes?

@@ +419,3 @@
>                                      mParentPerformance)
> +  // Traverse method from the PerformanceBase class.
> +  tmp->Traverse(cb);

Ditto.

@@ +777,5 @@
> +{}
> +
> +void
> +PerformanceBase::GetEntries(nsTArray<nsRefPtr<PerformanceEntry>>& aRetval)
> +{

I'm assuming most of this stuff is just moving the code.  If you are changing the implementation here as well, can you please point me to the specific places?  Thanks!

@@ +989,5 @@
> +  }
> +}
> +
> +void
> +PerformanceBase::Unlink()

This and Traverse will go away, hopefully.

::: dom/base/nsPerformance.h
@@ +394,5 @@
>  
>    virtual JSObject* WrapObject(JSContext *cx, JS::Handle<JSObject*> aGivenProto) override;
>  
>    // Performance WebIDL methods
> +  virtual DOMHighResTimeStamp Now() const override;

Please drop the virtual keyword.

@@ +420,5 @@
>  
>  private:
>    ~nsPerformance();
> +
> +  virtual nsISupports* GetAsISupports() override

Same, and in other occurrences below.

@@ +425,5 @@
> +  {
> +    return this;
> +  }
> +
> +  virtual void InsertUserEntry(PerformanceEntry* aEntry);

You should mark this as override too.

::: dom/base/test/test_performance_user_timing.js
@@ +1,1 @@
> +var steps = [

Also assuming this is just moving the code as well.

::: dom/webidl/Performance.webidl
@@ +29,5 @@
>    jsonifier;
>  };
>  
>  // http://www.w3.org/TR/performance-timeline/#sec-window.performance-attribute
> +[Exposed=(Window,Worker)]

Please send an intent to implement and ship.

::: dom/workers/Performance.cpp
@@ +70,5 @@
> +  if (aTime == 0) {
> +    return 0;
> +  }
> +
> +  return aTime - mWorkerPrivate->NowBaseTimeHighRes();

This is probably worth mentioning in the intent email.

::: dom/workers/Performance.h
@@ +43,5 @@
>      return nullptr;
>    }
>  
>    // WebIDL (public APIs)
> +  virtual DOMHighResTimeStamp Now() const override;

Please drop virtual in the appropriate places in this file too.
Attachment #8607468 - Flags: review?(ehsan) → review-
Thanks for the review. The changes I did in the code are:

1. I removed mWindow in nsPerformance and replaced it with GetOwner()

2. some MOZ_ASSERT for main-thread only methods (something that is not exposed to workers)

3. in the test I implemented a check for navigationStart

4. My patch uses DOMHighResTimeStamp in DeltaFromNavigationStart() and other methods instead DOMTimeMilliSec otherwise this code:

  return aTime - GetDOMTiming()->GetNavigationStart();

would return -0.something instead 0. This was an existing bug and we didn't have any test at all for it. I added some.

5. nsPerformance::IsEnabled()
Created attachment 8608013 [details] [diff] [review]
timing.patch
Attachment #8607468 - Attachment is obsolete: true
Attachment #8608013 - Flags: review?(ehsan)
Keywords: dev-doc-needed

Updated

3 years ago
Attachment #8608013 - Flags: review?(ehsan) → review+
Release Note Request (optional, but appreciated)
[Why is this notable]: It's an useful API to write benchmarks, tests and so on in workers.
[Suggested wording]: User Timing API exposed to workers
relnote-b2g: --- → ?
https://hg.mozilla.org/mozilla-central/rev/657edac53a66
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #29)
> > +  virtual void InsertUserEntry(PerformanceEntry* aEntry);
> 
> You should mark this as override too.

Looks like this review comment was missed -- the subclass version of this method (in nsPerformance) needs an 'override' annotation, or else clang 3.6 & newer complains.

Landed a followup to add the annotation, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/3ceaaa0aec90
Depends on: 1168411
Note added to:

https://developer.mozilla.org/en-US/docs/Web/API/Performance

Also added to:

https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers#APIs_available_in_workers

I have opened a new card on the MDN Trello board for documenting the User Timing/Performance Timeline APIs, which specify the currently-undocumented parts of Performance.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.