Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Maintain separate queues of GLES operations for each thread in the reactor #56573

Merged
auto-submit merged 4 commits into from
Nov 15, 2024
Merged

[Impeller] Maintain separate queues of GLES operations for each thread in the reactor #56573

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e233535
[Impeller] Maintain separate queues of GLES operations for each threa…
jason-simmons Nov 14, 2024
02b0c7d
test
jason-simmons Nov 14, 2024
9b2db7f
glFinish
jason-simmons Nov 15, 2024
328f4ca
fix proc table order
jason-simmons Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions impeller/renderer/backend/gles/reactor_gles.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,10 @@ bool ReactorGLES::RemoveWorker(WorkerID worker) {
}

bool ReactorGLES::HasPendingOperations() const {
auto thread_id = std::this_thread::get_id();

@gaaclarke gaaclarke Nov 14, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for future archeology and AIs: I was concerned about the overhead of this call, worrying it would require a syscall. I was wrong, Jason showed me that isn't the case. Pthread is keeping that info in user space.

Lock ops_lock(ops_mutex_);
return !ops_.empty();
auto it = ops_.find(thread_id);
return it != ops_.end() ? !it->second.empty() : false;
}

const ProcTableGLES& ReactorGLES::GetProcTable() const {
Expand Down Expand Up @@ -136,9 +138,10 @@ bool ReactorGLES::AddOperation(Operation operation) {
if (!operation) {
return false;
}
auto thread_id = std::this_thread::get_id();
{
Lock ops_lock(ops_mutex_);
ops_.emplace_back(std::move(operation));
ops_[thread_id].emplace_back(std::move(operation));
}
// Attempt a reaction if able but it is not an error if this isn't possible.
[[maybe_unused]] auto result = React();
Expand Down Expand Up @@ -191,10 +194,6 @@ bool ReactorGLES::React() {
}
TRACE_EVENT0("impeller", "ReactorGLES::React");
while (HasPendingOperations()) {
// Both the raster thread and the IO thread can flush queued operations.
// Ensure that execution of the ops is serialized.
Lock execution_lock(ops_execution_mutex_);

if (!ReactOnce()) {
return false;
}
Expand Down Expand Up @@ -279,10 +278,11 @@ bool ReactorGLES::FlushOps() {

// Do NOT hold the ops or handles locks while performing operations in case
// the ops enqueue more ops.
decltype(ops_) ops;
decltype(ops_)::mapped_type ops;
auto thread_id = std::this_thread::get_id();
{
Lock ops_lock(ops_mutex_);
std::swap(ops_, ops);
std::swap(ops_[thread_id], ops);
}
for (const auto& op : ops) {
TRACE_EVENT0("impeller", "ReactorGLES::Operation");
Expand Down
6 changes: 3 additions & 3 deletions impeller/renderer/backend/gles/reactor_gles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,9 @@ class ReactorGLES {

std::unique_ptr<ProcTableGLES> proc_table_;

Mutex ops_execution_mutex_;
mutable Mutex ops_mutex_;
std::vector<Operation> ops_ IPLR_GUARDED_BY(ops_mutex_);
std::map<std::thread::id, std::vector<Operation>> ops_ IPLR_GUARDED_BY(

@gaaclarke gaaclarke Nov 14, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to instead key this with EGLContext? It sounded like Chinmay and Jonah had use cases in mind that revolved around passing the context between multiple threads in a worker pool?

@jason-simmons jason-simmons Nov 14, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently EGL contexts are not passed between threads.

ReactorGLES does support calling AddOperation on a thread that does not have an active EGLContext. Using the thread ID as a key allows queueing of operations when the EGLContext is unavailable.

ops_mutex_);

// Make sure the container is one where erasing items during iteration doesn't
// invalidate other iterators.
Expand All @@ -280,7 +280,7 @@ class ReactorGLES {
bool can_set_debug_labels_ = false;
bool is_valid_ = false;

bool ReactOnce() IPLR_REQUIRES(ops_execution_mutex_);
bool ReactOnce();

bool HasPendingOperations() const;

Expand Down
1 change: 1 addition & 0 deletions lib/ui/painting/image_decoder_impeller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,7 @@ ImageDecoderImpeller::UnsafeUploadTextureToPrivate(
FML_DLOG(ERROR) << decode_error;
return std::make_pair(nullptr, decode_error);
}
command_buffer->WaitUntilScheduled();

@gaaclarke gaaclarke Nov 14, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a glFlush required here? Can you add a comment explaining why it's required. We aren't doing this elsewhere and it is going to add a performance hit to the decoder on opengles.

@jason-simmons jason-simmons Nov 14, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WaitUntilScheduled (which wraps glFlush) is required here. Without this, there is no assurance that the GPU will launch execution of the IO thread command buffer before the raster thread tries to access the texture.

Added a comment.

@gaaclarke gaaclarke Nov 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no assurance that the GPU will launch execution of the IO thread command buffer before the raster thread tries to access the texture.

If that's the case, are we sure we don't need a glFinish then? The glFlush is just making sure the commands are submitted to the GPU, but glFinish would make it synchronize on them actually completing. With a glFlush couldn't the raster thread potentially be using the texture before the data is finished being uploaded? Or maybe the driver is synchronizing on it if it isn't finished yet?

@jason-simmons jason-simmons Nov 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glFinish is expensive - I can see a major increase in latency when scrolling quickly through the Wonderous home screen pages on a Pixel 3. It takes several seconds for the app to catch up and render all pending images if each image decode does a glFinish.

However, IIUC the spec does not guarantee that a glFlush on the IO thread will ensure that the texture is safely usable when the raster thread consumes it on all GLES implementations.

Renamed CommandBuffer::WaitUntilScheduled to CommandBuffer::WaitUntilCompleted and changed it to call glFinish.

We should look into having GLES textures use glFenceSync/glWaitSync (flutter/flutter#158963)

@gaaclarke gaaclarke Nov 15, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes from the discord channel:


context->DisposeThreadLocalCachedResources();

Expand Down