Require a round-trip for Sync objects

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment)

Assignee

Description

Last year
We changed the spec to require this, like for Queries.
Comment hidden (mozreview-request)

Comment 2

Last year
mozreview-review
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

Last year
mozreview-review
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 on attachment 8955732 [details]
Bug 1442502 - Require event loop roundtrip for WebGLSync. -

changing the review status till the concerns are addressed
Attachment #8955732 - Flags: review?(kvark) → review-
Assignee

Comment 5

Last year
mozreview-review
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?
Attachment #8955732 - Flags: review?(jgilbert)
Assignee

Updated

Last year
Attachment #8955732 - Flags: review?(jgilbert) → review?(kvark)

Comment 6

Last year
mozreview-review
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.
Attachment #8955732 - Flags: review- → review?(kvark)
Assignee

Comment 7

Last year
mozreview-review
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.
Attachment #8955732 - Flags: review?(jgilbert)

Comment 8

Last year
mozreview-review
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!
Attachment #8955732 - Flags: review?(kvark) → review+
Attachment #8955732 - Flags: review?(kvark) → review+

Comment 9

Last year
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77acafe9c813
Require event loop roundtrip for WebGLSync. - r=kvark

Comment 10

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/77acafe9c813
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee

Updated

Last year
Attachment #8955732 - Flags: review?(jgilbert)
You need to log in before you can comment on or make changes to this bug.