Bug 89681 – [chromium] Allow impl thread to delete all allocated textures in TextureManager

• Component: New Bugs
• Status: RESOLVED INVALID

[Dana Jansens]

[chromium] Allow impl thread to delete all allocated textures in TextureManager

[Dana Jansens]

Created attachment 148849 [details]
Patch

[Dana Jansens]

The patch only modifies functions on TM that take TextureAllocator*, which means all these methods are accessed only from impl thread.

The departure here is that the deleteAllTexturesOnImpl can be called at any time on impl without main thread being blocked (it doesn't touch anything that is used by methods that don't take TextureAllocator* as input).

Then, when main/impl thread are synced, in beginFrame somewhere, one would call the syncTexturesWithImpl method.

[Dana Jansens]

Created attachment 148853 [details]
Patch

I think the visiblity stuff makes the previous code ok but it isn't obvious that it's threadsafe. I looked at your patch again James to see how you're calling the TextureAllocator and TextureManager and mimicing that a bit better. So the Proxy can decide to tell the TM that everything is deleted or not as you currently do. This just lets us do the texture id tracking/free here instead of adding it to TextureAllocator, which was the intention.

[James Robinson]

Comment on attachment 148853 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148853&action=review

> Source/WebCore/platform/graphics/chromium/TextureManager.h:92
> +    // notifyDeletedAllTexturesOnImpl() must be called between calling this and allocating new
> +    // textures. This method does not require main thread to be blocked in order to be used.

I'm not a fan of making the texture manager itself have to know about different threads.  Currently threading concerns are managed by the caller

[Dana Jansens]

Comment on attachment 148853 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148853&action=review

>> Source/WebCore/platform/graphics/chromium/TextureManager.h:92
>> +    // textures. This method does not require main thread to be blocked in order to be used.
> 
> I'm not a fan of making the texture manager itself have to know about different threads.  Currently threading concerns are managed by the caller

The caller does manage the thread safety here also, by making notifyDeletedAllTexturesOnImpl() the first thing it calls on main thread, after using deleteAllTexturesOnImpl().

TM allows this by using a data structure that is only touched during allocate/free (and these are only done on impl thread). Does that mean it knows about different threads?

I don't really like using TextureAllocator as another texture manager, which is why I propose this, but maybe there are other solutions. I'll bring the discussion about TextureAllocator over there.

[James Robinson]

I don't really understand what the problem is that you are trying to solve.

[Dana Jansens]

It just seems like we're distributing some of the role of TextureManager to TextureAllocator, and we already have so many classes that create/delete/manage lifetime of textures in some way, making yet another class with these types of responsibilities seems to be cluttering things. Maybe I've just been staring at this stuff too long, but I'd like to have these responsibilities in fewer classes instead of more.

[Dana Jansens]

this is invalidated by PTM landing.

[Adrienne Walker]

Comment on attachment 148853 [details]
Patch

Taking review flag off to clear from the queue, since this is marked as invalid.