Added the ability to make untracked opengles handles (migrated textures)

[gaaclarke]

issue: flutter/flutter#159745

This tweaks ReactorGLES so that one can easily opt out of it's mutex/hash map for cases where we can statically reason about the safety of doing so. The goal here was to make migration of existing code really easy to do. It may be in the future that everything is an untracked handle? We can move there in baby steps.

Potential follow up PRs:

• Move Pipeline to use untracked handles
• Move DeviceBufferGLES to use untracked handles
• Add a new method to synchronously delete untracked handles
• Start storing handles to be deleted in its own vector, so handles_ doesn't need to be used for deleting untracked handles

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[jonahwilliams]

I believe textures can be created on any thread. To make the texture object manage its own handle, you'd lazily initialize it in the InitializeContentsIfNecessary function

[gaaclarke]

That's what's happening here. CreateUntrackedHandle creates the texture synchronously.

[jonahwilliams]

If it creates it synchronously, then we have to be careful not to create texture objects on threads other than raster/IO. but if you defer construction then the existing lifecycle mechanisms manage that for you.

[gaaclarke]

That's what the assertion that the thread reacts is maintaining. I don't think we have a use case where we have to create textures on a thread other than the io/raster thread.

[gaaclarke]

ReactorGLES::CreateHandle is already creating these synchronously too, so there is no change in the behavior here.

[jonahwilliams]

It only creates it immediately if CanReactOnCurrentThread is true:

https://github.com/flutter/engine/blob/main/impeller/renderer/backend/gles/reactor_gles.cc#L208-L210

[gaaclarke]

It only creates it immediately if CanReactOnCurrentThread is true:

Right, but we assert CanReactOnCurrentThread so it's the same. I ran wonderous with this patch ( #56927 (comment) ) and the assert was never hit. It seems like a feature we may not even be using.

[jonahwilliams]

it might be getting used by flutter gpu, but if we're not hitting the assert anywhere that is good enough for me

[gaaclarke]

@jonahwilliams I'm putting this into review now. It does a portion of what we were talking about. I think this moves us in the right direction to get there. Check out the description for some idea about where we go after this PR.

[gaaclarke]

I checked wonderous with the following additions:

diff --git a/impeller/renderer/backend/gles/device_buffer_gles.cc b/impeller/renderer/backend/gles/device_buffer_gles.cc
index f4caaf5af8..aaed494cab 100644
--- a/impeller/renderer/backend/gles/device_buffer_gles.cc
+++ b/impeller/renderer/backend/gles/device_buffer_gles.cc
@@ -17,7 +17,7 @@ DeviceBufferGLES::DeviceBufferGLES(DeviceBufferDescriptor desc,
                                    std::shared_ptr<Allocation> backing_store)
     : DeviceBuffer(desc),
       reactor_(std::move(reactor)),
-      handle_(reactor_ ? reactor_->CreateHandle(HandleType::kBuffer)
+      handle_(reactor_ ? reactor_->CreateUntrackedHandle(HandleType::kBuffer)
                        : HandleGLES::DeadHandle()),
       backing_store_(std::move(backing_store)) {}
 
diff --git a/impeller/renderer/backend/gles/unique_handle_gles.cc b/impeller/renderer/backend/gles/unique_handle_gles.cc
index ddf4081791..07b4bf00ef 100644
--- a/impeller/renderer/backend/gles/unique_handle_gles.cc
+++ b/impeller/renderer/backend/gles/unique_handle_gles.cc
@@ -11,7 +11,7 @@ namespace impeller {
 UniqueHandleGLES::UniqueHandleGLES(ReactorGLES::Ref reactor, HandleType type)
     : reactor_(std::move(reactor)) {
   if (reactor_) {
-    handle_ = reactor_->CreateHandle(type);
+    handle_ = reactor_->CreateUntrackedHandle(type);
   }
 }

Things are looking good to use this everywhere and all the overhead of reactor was eliminated from the profile.

[jonahwilliams]

LGTM

[gaaclarke]

Chatted with @jonahwilliams offline. This approach is looking good but I need to make sure I didn't break debug naming. We can punt on synchronous deletion the profiles after pipelines, buffers and textures are moved over is looking good.

[chinmaygarde]

Does this put additional burden on the caller to ensure that texture handles in the OpenGL ES backend are created or destroyed on a specific thread?

[gaaclarke]

Does this put additional burden on the caller to ensure that texture handles in the OpenGL ES backend are created or destroyed on a specific thread?

It has the burden that a call to create must happen on a thread that reacts. For deletion it goes through the same flow (so can happen on any thread). The major costs were GetHandle(), not the creation and deletion.

[chinmaygarde]

In that case, this will be an issue for the OpenGL Flutter GPU runtime. I'll file a bug to sort out the threading implications for Flutter GPU like usages.

[gaaclarke]

In that case, this will be an issue for the OpenGL Flutter GPU runtime. I'll file a bug to sort out the threading implications for Flutter GPU like usages.

Here's a model on how to do it: a289daa

You track the untracked handle only long enough to perform the operation. That makes things like naming a item require synchronization, but no hash table lookup. For creation we'd need a future so that we can make sure subsequent accesses to the name require no hash table lookup or synchronization.

[gaaclarke]

@chinmaygarde also note I haven't closed any existing code paths yet. the slow fully tracked way is still available. We'll probably want to move everything to untracked at some point but have 2 entrypoints that are threadsafe vs not threadsafe, not tracked vs not tracked.