Add gl::MainThreadSyncObject

NEW
Unassigned

Status

()

5 years ago
2 months ago

People

(Reporter: vlad, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 8351262 [details] [diff] [review]
implement MainThreadSyncObject

This is a helper class for ARB_sync.  The difficulty is that GLSync objects aren't reusable.  A new one has to be created each time.  But if we're synchronizing between different threads, we need a way to send that object to another thread in a safe way.  This helper does that.

It also ensures that when any thread does a wait, we can't swap out the sync object until the wait is done.
Deletion also always happens on the main thread.

What do you guys think?
Attachment #8351262 - Flags: review?(jgilbert)
Attachment #8351262 - Flags: review?(bjacob)
Comment on attachment 8351262 [details] [diff] [review]
implement MainThreadSyncObject

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

feedback+; you'll want :jgilbert only for the review, as I've never worked much with fences and he has.

Note that I just removed these GLContextUtils.* files. You'll want to create a pair of new files for this.

::: gfx/gl/GLContextUtils.cpp
@@ +68,5 @@
> +
> +void *
> +MainThreadSyncObject::AtomicExchangeSync(void *aNewSync)
> +{
> +    // uh, is there some kind of InterlockedExchange we could use here?

Yup, search for 'exchange' in mfbt/Atomics.h

::: gfx/gl/GLContextUtils.h
@@ +30,5 @@
> +{
> +public:
> +    // context for creation/deletion
> +    MainThreadSyncObject(GLContext *cx = nullptr);
> +    void SetMainThreadContext(GLContext *cx) {

What is the use case for creating without a context and setting the context later?
Also, you could assert that the passed context really is main-thread (check GLContext::mOwningThread).

@@ +48,5 @@
> +protected:
> +    void *AtomicExchangeSync(void *newSync);
> +    nsRefPtr<GLContext> mMainThreadGLContext;
> +    Mutex mMutex;
> +    void *mSync;

Why is this a void* and not a GLsync, which we already seem to have as a typedef in GLTypes.h ?
Attachment #8351262 - Flags: review?(bjacob) → feedback+
Why does this focus on the main thread? We're just about to pull everything off-main-thread, so adding a main-thread-only class now doesn't seem that useful?
Flags: needinfo?(vladimir)
Comment on attachment 8351262 [details] [diff] [review]
implement MainThreadSyncObject

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

Removing review request pending needinfo response.
Attachment #8351262 - Flags: review?(jgilbert)
Flags: needinfo?(vladimir)
You need to log in before you can comment on or make changes to this bug.