1251390 - Make timer queries available at the appropriate time

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make timer queries available at the appropriate time

This is similar to what we do for other queries

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make timer queries available at the appropriate time

Here's a version that uses a lambda which is much nicer.

Comment 2

[Kelsey Gilbert [:jgilbert]]

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?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

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

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

Indeed

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make timer queries available at the appropriate time

This time with more sanity.

Comment 5

[Kelsey Gilbert [:jgilbert]]

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.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(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;

Comment 7

[Kelsey Gilbert [:jgilbert]]

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.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(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.

Comment 10

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/406908b6e015

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5195eef18d6

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5195eef18d6