Make timer queries available at the appropriate time

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jrmuizel, Assigned: jgilbert)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

3 years ago
This is similar to what we do for other queries
Reporter

Comment 1

3 years ago
Here's a version that uses a lambda which is much nicer.
Attachment #8723742 - Attachment is obsolete: true
Attachment #8723768 - Flags: review?(jgilbert)
Reporter

Updated

3 years ago
Blocks: 1251375
Whiteboard: [gfx-noted]
Assignee

Comment 2

3 years ago
Comment on attachment 8723768 [details] [diff] [review]
Make timer queries available at the appropriate time

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

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
@@ +121,5 @@
>    mContext->GL()->fEndQuery(target);
>    mActiveQuery = nullptr;
> +
> +  RefPtr<WebGLExtensionDisjointTimerQuery> self = this;
> +  NS_DispatchToCurrentThread(NS_NewRunnableFunction([self] { self->mCanBeAvailable = true; }));

Surely this should be based on the query, not the extension?
Assignee

Updated

3 years ago
Flags: needinfo?(jmuizelaar)
Reporter

Comment 3

3 years ago
Comment on attachment 8723768 [details] [diff] [review]
Make timer queries available at the appropriate time

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

Indeed
Attachment #8723768 - Flags: review?(jgilbert) → review-
Reporter

Comment 4

3 years ago
This time with more sanity.
Attachment #8723768 - Attachment is obsolete: true
Flags: needinfo?(jmuizelaar)
Assignee

Comment 5

3 years ago
Comment on attachment 8725342 [details] [diff] [review]
Make timer queries available at the appropriate time

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

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
@@ +123,3 @@
>    mActiveQuery = nullptr;
> +
> +  NS_DispatchToCurrentThread(NS_NewRunnableFunction([query] { query->MarkCanBeAvailable(); }));

I'm a little surprised this works. I would actually appreciate having this pulled out into a static function.

::: dom/canvas/WebGLTimerQuery.h
@@ +24,5 @@
>    void Delete();
>  
>    bool HasEverBeenBound() const { return mTarget != LOCAL_GL_NONE; }
> +  bool CanBeAvailable() const { return mCanBeAvailable; }
> +  void MarkCanBeAvailable() { mCanBeAvailable = true; }

You can also just make the extension class a friend, and skip the getter.
Attachment #8725342 - Flags: review+
Reporter

Comment 6

3 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 8725342 [details] [diff] [review]
> Make timer queries available at the appropriate time
> 
> Review of attachment 8725342 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
> @@ +123,3 @@
> >    mActiveQuery = nullptr;
> > +
> > +  NS_DispatchToCurrentThread(NS_NewRunnableFunction([query] { query->MarkCanBeAvailable(); }));
> 
> I'm a little surprised this works. I would actually appreciate having this
> pulled out into a static function.

Which part would be static:

How about this instead?

void WebGLTimerQuery::QueueAvailability() {
  RefPtr<WebGLTimerQuery> self = this;
  NS_DispatchToCurrentThread(NS_NewRunnableFunction([self] { self->mCanBeAvailable = true; }));
}

and the caller would just be:

mActiveQuery->QueueAvailability();
mActiveQuery = nullptr;
Reporter

Updated

3 years ago
Flags: needinfo?(jmuizelaar)
Reporter

Updated

3 years ago
Flags: needinfo?(jmuizelaar) → needinfo?(jgilbert)
Assignee

Comment 7

3 years ago
It's the NS_NewRunnableFucntion(<lambda>) that's inscrutable to me. Is this the standard usage pattern? It's hard for me to figure out if this is safe, etc.
Flags: needinfo?(jgilbert)
Assignee

Updated

3 years ago
Assignee: nobody → jgilbert
Reporter

Comment 8

3 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> It's the NS_NewRunnableFucntion(<lambda>) that's inscrutable to me. Is this
> the standard usage pattern? It's hard for me to figure out if this is safe,
> etc.

Yeah, NS_NewRunnableFunction just wraps a lambda in a newly allocated nsRunnableFunction.
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> It's the NS_NewRunnableFucntion(<lambda>) that's inscrutable to me. Is this
> the standard usage pattern? It's hard for me to figure out if this is safe,
> etc.

[drive-by] More lambdas please; we have way too many one-off static functions often declared/defined far away from their actual one use case.  That was only because we didn't have nice things in the dark ages.
Assignee

Comment 10

3 years ago
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> (In reply to Jeff Gilbert [:jgilbert] from comment #7)
> > It's the NS_NewRunnableFucntion(<lambda>) that's inscrutable to me. Is this
> > the standard usage pattern? It's hard for me to figure out if this is safe,
> > etc.
> 
> [drive-by] More lambdas please; we have way too many one-off static
> functions often declared/defined far away from their actual one use case. 
> That was only because we didn't have nice things in the dark ages.

I am quite pro-lambda in the general case. It is this particular case where I had concerns.

However, if this is the normal thing to do, this is fine.
Backed out for webgl failures.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1992eff1a02d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=406908b6e015
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23540431&repo=mozilla-inbound

03:01:34     INFO -  764 INFO TEST-PASS | dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html | Query is no longer valid after deletion.
03:01:34     INFO -  765 INFO TEST-PASS | dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html | Time elapsed must be at least 30 bits to hold at least 1 second of timing.
03:01:34     INFO -  766 INFO TEST-UNEXPECTED-FAIL | dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html | Timestamp query should be available immediately after flush and event loop tick.
03:01:34     INFO -      doTest/</<@dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html:61:11
03:01:34     INFO -      setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:621:12
03:01:34     INFO -      defer@dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html:17:5
03:01:34     INFO -      doTest/<@dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html:60:9
03:01:34     INFO -      setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:621:12
03:01:34     INFO -      defer@dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html:17:5
03:01:34     INFO -      doTest@dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html:45:3
Flags: needinfo?(jmuizelaar)

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5195eef18d6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter

Updated

3 years ago
Flags: needinfo?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.