Closed Bug 1060085 Opened 8 years ago Closed 8 years ago

Add Thread-local versions of ShSurf::Fence/Poll/Wait

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(2 files, 2 obsolete files)

We get access to some better synchronization opportunities if we have thread-local versions of ShSurf::Poll and Wait.

For instance, if we add a frame of delay, but call poll/wait on the content thread, we can wait until then to readback, which may allow for better throughput than doing readback during Fence().

Also, NV_fence for ANGLE has to happen on the same GLContext, so we can only use fTestFence and fFinishFence on the same thread we make the fence on.
Comment on attachment 8480887 [details] [diff] [review]
patch 1: Add entrypoints and add NV_fence support to ANGLE ShSurfs

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

::: gfx/gl/SharedSurface.h
@@ +86,5 @@
>  
> +    // Use these if you can. They can only be called from the Content
> +    // thread, though!
> +    virtual void Fence_ThreadLocal() {
> +        Fence();

Should we assert the thread it's being called from? I'm not a huge fan of _ThreadLocal(). Maybe _ContentThread, or something. I don't know, but I also don't think it warrants an r-.
Attachment #8480887 - Flags: review?(dglastonbury) → review+
Comment on attachment 8480891 [details] [diff] [review]
patch 2: Add thread-local Fence/Poll/Wait to Basic ShSurf

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

::: gfx/gl/SharedSurfaceGL.cpp
@@ +72,5 @@
>      , mTex(tex)
>      , mFB(0)
> +    , mIsDataCurrent(false)
> +#ifdef DEBUG
> +    , mParentThread(NS_GetCurrentThread())

Should this be mOwningThread, perhaps?  Parent implies hierarchy to me (object-to-object). In this case, I think a thread "owns" the fence/surf/etc.
Attachment #8480891 - Flags: review?(dglastonbury) → review+
I changed the name and made it assert without having to add asserts to each implementation.
Attachment #8480887 - Attachment is obsolete: true
Attachment #8481575 - Flags: review?(dglastonbury)
This is just a de-bitrotted version of the previous r+.
r=kamidphish
Attachment #8480891 - Attachment is obsolete: true
Attachment #8481576 - Flags: review+
Attachment #8481575 - Flags: review?(dglastonbury) → review+
https://hg.mozilla.org/mozilla-central/rev/d1bc9a5c17ff
https://hg.mozilla.org/mozilla-central/rev/9d4fc9f2d5d1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.