1386080 - Make DrawTarget thread safe RefCountable

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Mason Chang [Inactive] [:mchang]]

With OMTP, we hold RefPtrs to DrawTarget off the main thread. This could potentially be the last reference. Make DrawTarget thread safe refcountable.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Make DrawTargets threadsafe refcounted

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Make DrawTargets threadsafe refcounted

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Make DrawTargets threadsafe refcounted

Comment 4

[Matt Woodrow (:mattwoodrow)]

What if the underlying buffer of the DT (say, a shmem) isn't something the DT can hold a threadsafe reference to?

Comment 5

[Mason Chang [Inactive] [:mchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> What if the underlying buffer of the DT (say, a shmem) isn't something the
> DT can hold a threadsafe reference to?

Well, technically the TextureClient owns the underlying buffer of the DT. We keep that alive on the main thread and so the underlying buffer should be released on the main thread. Same with the DT since the TextureClient should own a reference to it as well. Technically we could just hold draw pointers to the DTs with OMTP, but I'd rather not just for safety.

Comment 6

[Bas Schouten (:bas.schouten)]

Why aren't we using AtomicRefCounted rather than changing how the whole thing works?

Comment 7

[Mason Chang [Inactive] [:mchang]]

(In reply to Bas Schouten (:bas.schouten) from comment #6)
> Why aren't we using AtomicRefCounted rather than changing how the whole
> thing works?

Hmm one definition is in the js directory and it seems like we use it. The one in MFBT says don't use it [1] and use the macro instead. Is there something else you meant?


[1] http://searchfox.org/mozilla-central/source/mfbt/RefCounted.h#198

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Mason Chang [:mchang] from comment #7)
> (In reply to Bas Schouten (:bas.schouten) from comment #6)
> > Why aren't we using AtomicRefCounted rather than changing how the whole
> > thing works?
> 
> Hmm one definition is in the js directory and it seems like we use it. The
> one in MFBT says don't use it [1] and use the macro instead. Is there
> something else you meant?
> 
> 
> [1] http://searchfox.org/mozilla-central/source/mfbt/RefCounted.h#198

That's what I meant, but since we avoid dependencies on anything but MFBT (and I think we still should), I think AtomicRefCounted, in line with using RefCounted (which you're not 'supposed to use' either), which we do in all other classes in Moz2D, seems like the right way to go.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Make DrawTargets AtomicRefcounted

Comment 10

[Mason Chang [Inactive] [:mchang]]

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb7e8852805146bb9b90a40319bfccb5351fb578

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c587c3e5de
Make DrawTarget thread safe refcountable. r=bas

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46c587c3e5de