Skip to content

Commit

Permalink
Smaller reactor handle locks (#56768)
Browse files Browse the repository at this point in the history
This reduces the amount of work that is happening inside the read/write
mutex for tracking handles in the Reactor.
1) Now gl cleanup calls happen outside of the write lock
1) Now setting gl debug texts are out of the write lock
1) Now creating objects with CreateHandle are out of the write lock

Jonah was seeing considerable time in the rwmutex, this should alleviate
some of that.


![setlabel](https://github.com/user-attachments/assets/6ebe247b-76f4-457f-b28b-70ddcd5615a8)

issue: flutter/flutter#159177

tests: no functional change, just performance

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] 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.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
  • Loading branch information
gaaclarke authored Nov 23, 2024
1 parent 7f160b9 commit 7231bf5
Showing 1 changed file with 45 additions and 29 deletions.
74 changes: 45 additions & 29 deletions impeller/renderer/backend/gles/reactor_gles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -203,14 +203,15 @@ HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) {
if (new_handle.IsDead()) {
return HandleGLES::DeadHandle();
}
WriterLock handles_lock(handles_mutex_);

std::optional<ReactorGLES::GLStorage> gl_handle;
if (external_handle != GL_NONE) {
gl_handle = ReactorGLES::GLStorage{.handle = external_handle};
} else if (CanReactOnCurrentThread()) {
gl_handle = CreateGLHandle(GetProcTable(), type);
}

WriterLock handles_lock(handles_mutex_);
handles_[new_handle] = LiveHandle{gl_handle};
return new_handle;
}
Expand Down Expand Up @@ -266,41 +267,56 @@ bool ReactorGLES::ReactOnce() {
bool ReactorGLES::ConsolidateHandles() {
TRACE_EVENT0("impeller", __FUNCTION__);
const auto& gl = GetProcTable();
WriterLock handles_lock(handles_mutex_);
std::vector<HandleGLES> handles_to_delete;
for (auto& handle : handles_) {
// Collect dead handles.
if (handle.second.pending_collection) {
// This could be false if the handle was created and collected without
// use. We still need to get rid of map entry.
if (handle.second.name.has_value()) {
CollectGLHandle(gl, handle.first.type, handle.second.name.value());
std::vector<std::tuple<HandleGLES, std::optional<GLStorage>>>
handles_to_delete;
std::vector<std::tuple<DebugResourceType, GLint, std::string>>
handles_to_name;
{
WriterLock handles_lock(handles_mutex_);
for (auto& handle : handles_) {
// Collect dead handles.
if (handle.second.pending_collection) {
handles_to_delete.push_back(
std::make_tuple(handle.first, handle.second.name));
continue;
}
handles_to_delete.push_back(handle.first);
continue;
}
// Create live handles.
if (!handle.second.name.has_value()) {
auto gl_handle = CreateGLHandle(gl, handle.first.type);
if (!gl_handle) {
VALIDATION_LOG << "Could not create GL handle.";
return false;
// Create live handles.
if (!handle.second.name.has_value()) {
auto gl_handle = CreateGLHandle(gl, handle.first.type);
if (!gl_handle) {
VALIDATION_LOG << "Could not create GL handle.";
return false;
}
handle.second.name = gl_handle;
}
handle.second.name = gl_handle;
}
// Set pending debug labels.
if (handle.second.pending_debug_label.has_value() &&
handle.first.type != HandleType::kFence) {
if (gl.SetDebugLabel(ToDebugResourceType(handle.first.type),
handle.second.name.value().handle,
handle.second.pending_debug_label.value())) {
// Set pending debug labels.
if (handle.second.pending_debug_label.has_value() &&
handle.first.type != HandleType::kFence) {
handles_to_name.push_back(std::make_tuple(
ToDebugResourceType(handle.first.type),
handle.second.name.value().handle,
std::move(handle.second.pending_debug_label.value())));
handle.second.pending_debug_label = std::nullopt;
}
}
for (const auto& handle_to_delete : handles_to_delete) {
handles_.erase(std::get<0>(handle_to_delete));
}
}
for (const auto& handle_to_delete : handles_to_delete) {
handles_.erase(handle_to_delete);

for (const auto& handle : handles_to_name) {
gl.SetDebugLabel(std::get<0>(handle), std::get<1>(handle),
std::get<2>(handle));
}
for (const auto& handle : handles_to_delete) {
const std::optional<GLStorage>& storage = std::get<1>(handle);
// This could be false if the handle was created and collected without
// use. We still need to get rid of map entry.
if (storage.has_value()) {
CollectGLHandle(gl, std::get<0>(handle).type, storage.value());
}
}

return true;
}

Expand Down

0 comments on commit 7231bf5

Please sign in to comment.