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
Prev Previous commit
Next Next commit
test
  • Loading branch information
@jason-simmons
jason-simmons committed Nov 14, 2024
commit 02b0c7de0afdd7dc2832efc5ffee334a0baa12cb
33 changes: 33 additions & 0 deletions impeller/renderer/backend/gles/test/reactor_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#include <memory>
#include "flutter/fml/synchronization/waitable_event.h"
#include "flutter/testing/testing.h" // IWYU pragma: keep
#include "gtest/gtest.h"
#include "impeller/renderer/backend/gles/handle_gles.h"
Expand Down Expand Up @@ -60,5 +61,37 @@ TEST(ReactorGLES, DeletesHandlesDuringShutdown) {
calls.end());
}

TEST(ReactorGLES, PerThreadOperationQueues) {
auto mock_gles = MockGLES::Init();
ProcTableGLES::Resolver resolver = kMockResolverGLES;
auto proc_table = std::make_unique<ProcTableGLES>(resolver);
auto worker = std::make_shared<TestWorker>();
auto reactor = std::make_shared<ReactorGLES>(std::move(proc_table));
reactor->AddWorker(worker);

bool op1_called = false;
EXPECT_TRUE(
reactor->AddOperation([&](const ReactorGLES&) { op1_called = true; }));

fml::AutoResetWaitableEvent event;
bool op2_called = false;
std::thread thread([&] {
EXPECT_TRUE(
reactor->AddOperation([&](const ReactorGLES&) { op2_called = true; }));
event.Wait();
EXPECT_TRUE(reactor->React());
});

// Reacting on the main thread should only run the main thread's operation.
EXPECT_TRUE(reactor->React());
EXPECT_TRUE(op1_called);
EXPECT_FALSE(op2_called);

// Reacting on the second thread will run the second thread's operation.
event.Signal();
thread.join();
EXPECT_TRUE(op2_called);
}

} // namespace testing
} // namespace impeller
3 changes: 3 additions & 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,9 @@ ImageDecoderImpeller::UnsafeUploadTextureToPrivate(
FML_DLOG(ERROR) << decode_error;
return std::make_pair(nullptr, decode_error);
}

// Flush the pending command buffer to ensure that its output becomes visible
// to the raster thread.
command_buffer->WaitUntilScheduled();
Copy link
Member

@gaaclarke gaaclarke Nov 14, 2024

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.

Copy link
Member Author

@jason-simmons jason-simmons Nov 14, 2024

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.

Copy link
Member

@gaaclarke gaaclarke Nov 14, 2024 •
edited
Loading

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?

Copy link
Member Author

@jason-simmons jason-simmons Nov 15, 2024

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)

Copy link
Member

@gaaclarke gaaclarke Nov 15, 2024

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