1442502 - Require a round-trip for Sync objects

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement, P1)

Description

[Kelsey Gilbert [:jgilbert]]

We changed the spec to require this, like for Queries.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1442502 - Require event loop roundtrip for WebGLSync. -

Review commit: https://reviewboard.mozilla.org/r/224816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224816/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8955732 [details]
Bug 1442502 - Require event loop roundtrip for WebGLSync. -

https://reviewboard.mozilla.org/r/224816/#review230916


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/canvas/WebGLContext.cpp:2483
(Diff revision 1)
> +    , mWebGL(webgl)
> +{
> +    mWebGL->mAvailabilityRunnable = this;
> +}
> +
> +webgl::AvailabilityRunnable::~AvailabilityRunnable()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Comment 3

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8955732 [details]
Bug 1442502 - Require event loop roundtrip for WebGLSync. -

https://reviewboard.mozilla.org/r/224816/#review230994

I don't feel confident putting a seal of approval here, missing a bit of context (that WebGL WG has been brewing). Left a few questions to clarify.

::: dom/canvas/WebGLContext.cpp:236
(Diff revision 1)
>      mQuerySlot_TimeElapsed = nullptr;
>  
>      mIndexedUniformBufferBindings.clear();
>  
> +    if (mAvailabilityRunnable) {
> +        mAvailabilityRunnable->Run();

shouldn't we also destroy this instance? otherwise it will keep holding `RefPtr<Context>`

::: dom/canvas/WebGLContext.cpp:2480
(Diff revision 1)
> +
> +webgl::AvailabilityRunnable::AvailabilityRunnable(WebGLContext* const webgl)
> +    : Runnable("webgl::AvailabilityRunnable")
> +    , mWebGL(webgl)
> +{
> +    mWebGL->mAvailabilityRunnable = this;

this indirection looks quite obscure. Could you explain why AvailabilityRunnable manages its own connection to the context and not the other way around?

Comment 4

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8955732 [details]
Bug 1442502 - Require event loop roundtrip for WebGLSync. -

changing the review status till the concerns are addressed

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8955732 [details]
Bug 1442502 - Require event loop roundtrip for WebGLSync. -

https://reviewboard.mozilla.org/r/224816/#review231082

::: dom/canvas/WebGLContext.cpp:236
(Diff revision 1)
>      mQuerySlot_TimeElapsed = nullptr;
>  
>      mIndexedUniformBufferBindings.clear();
>  
> +    if (mAvailabilityRunnable) {
> +        mAvailabilityRunnable->Run();

::Run() nulls the RefPtr.

::: dom/canvas/WebGLContext.cpp:2480
(Diff revision 1)
> +
> +webgl::AvailabilityRunnable::AvailabilityRunnable(WebGLContext* const webgl)
> +    : Runnable("webgl::AvailabilityRunnable")
> +    , mWebGL(webgl)
> +{
> +    mWebGL->mAvailabilityRunnable = this;

It's nice that if it nulls its var, it also assigns its var. (Since we null this var in ::Run())

::: dom/canvas/WebGLContext.cpp:2483
(Diff revision 1)
> +    , mWebGL(webgl)
> +{
> +    mWebGL->mAvailabilityRunnable = this;
> +}
> +
> +webgl::AvailabilityRunnable::~AvailabilityRunnable()

Really? It doesn't handle assert-only dtors?

Comment 6

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8955732 [details]
Bug 1442502 - Require event loop roundtrip for WebGLSync. -

https://reviewboard.mozilla.org/r/224816/#review231096

A few more questions...

::: dom/canvas/WebGLContext.cpp:236
(Diff revision 1)
>      mQuerySlot_TimeElapsed = nullptr;
>  
>      mIndexedUniformBufferBindings.clear();
>  
> +    if (mAvailabilityRunnable) {
> +        mAvailabilityRunnable->Run();

And this is the only place where `Run` is actually called? (`DestroyResourcesAndContext`) I assume deriving `Runnable` forces it to also run on an event loop or something, hence the need to store a pointer back to the context?

::: dom/canvas/WebGLContext.cpp:2495
(Diff revision 1)
> +webgl::AvailabilityRunnable::Run()
> +{
> +    for (const auto& cur : mQueries) {
> +        cur->mCanBeAvailable = true;
> +    }
> +    mQueries.clear();

This also appears to be different from the previous logic for queries, despite the commit message implying that the fix is intended to affect sync objects only.
The old code didn't clear the query, which I assume would affect the behavior of the same runnable getting run twice.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8955732 [details]
Bug 1442502 - Require event loop roundtrip for WebGLSync. -

https://reviewboard.mozilla.org/r/224816/#review231120

::: dom/canvas/WebGLContext.cpp:236
(Diff revision 1)
>      mQuerySlot_TimeElapsed = nullptr;
>  
>      mIndexedUniformBufferBindings.clear();
>  
> +    if (mAvailabilityRunnable) {
> +        mAvailabilityRunnable->Run();

Run is also called when the Runnable is triggered in the event loop we enqueue it in.

::: dom/canvas/WebGLContext.cpp:2495
(Diff revision 1)
> +webgl::AvailabilityRunnable::Run()
> +{
> +    for (const auto& cur : mQueries) {
> +        cur->mCanBeAvailable = true;
> +    }
> +    mQueries.clear();

Right, it seems like previously, calling Run twice would trigger multiple times, which is not the behavior we want.

Comment 8

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8955732 [details]
Bug 1442502 - Require event loop roundtrip for WebGLSync. -

https://reviewboard.mozilla.org/r/224816/#review231160

thanks for clarifications, ship it!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77acafe9c813
Require event loop roundtrip for WebGLSync. - r=kvark

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/77acafe9c813