[Impeller] one descriptor pool per frame. by jonahwilliams · Pull Request #55939 · flutter/engine

jonahwilliams · 2024-10-17T23:48:05Z

Creating descriptor pools is expensive, no need to have more than one per frame.

The lifecycle of the descriptor pool is exactly the same as the cmd pool, which indicates that we can probably do some consolidation/refactoring in the future.

Fixes flutter/flutter#157115

jonahwilliams · 2024-10-18T00:02:48Z

impeller/renderer/backend/vulkan/command_buffer_vk.h

                  std::weak_ptr<const DeviceHolderVK> device_holder,
-                  std::shared_ptr<TrackedObjectsVK> tracked_objects,
-                  std::shared_ptr<FenceWaiterVK> fence_waiter);
+                  std::shared_ptr<TrackedObjectsVK> tracked_objects);


Fence waiter was unused.

jonahwilliams · 2024-10-18T01:26:08Z

impeller/renderer/backend/vulkan/command_pool_vk.cc

 }

-// Associates a resource with a thread and context.
-using CommandPoolMap =


moved definition to header

jonahwilliams · 2024-10-18T01:26:23Z

impeller/renderer/backend/vulkan/command_pool_vk.cc

 //    available for reuse ("recycle").
 static thread_local std::unique_ptr<CommandPoolMap> tls_command_pool_map;

+struct WeakThreadLocalData {


The thread local data now holds both the cmd pool and desc pool

jonahwilliams · 2024-10-18T16:15:14Z

impeller/renderer/backend/vulkan/command_pool_vk.cc

-      if (!pool) {
-        continue;
+    for (auto& [command_pool, desc_pool] : found->second) {
+      const auto& strong_pool = command_pool.lock();


We don't actually need to do anything with the descriptor pool here, I just added it for symmetry....

jonahwilliams · 2024-10-18T19:18:45Z

reason for revert: failing on scenario test

This reverts commit 42c2a40.

Reverts: #55939 Initiated by: jonahwilliams Reason for reverting: failing on scenario test Original PR Author: jonahwilliams Reviewed By: {matanlurey} This change reverts the following previous change: Creating descriptor pools is expensive, no need to have more than one per frame. The lifecycle of the descriptor pool is exactly the same as the cmd pool, which indicates that we can probably do some consolidation/refactoring in the future. Fixes flutter/flutter#157115

flutter/engine@76d310e...dd37446 2024-10-19 robert.ancell@canonical.com Move keymap from FlKeyboardViewDelegate to FlKeyboardManager (flutter/engine#55942) 2024-10-19 jonahwilliams@google.com [UI] fix scene builder parameter naming. (flutter/engine#55969) 2024-10-19 chris@bracken.jp iOS: Improve thread safety of first frame callback (flutter/engine#55966) 2024-10-18 chris@bracken.jp iOS: Fix flaky tests (remove timeouts) (flutter/engine#55961) 2024-10-18 jonahwilliams@google.com [Impeller] allocate the impeller onscreen texture from the render target cache. (flutter/engine#55943) 2024-10-18 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 9F_NaKPd2twhbPwP7... to tNQZ8d5mRYpe3--lk... (flutter/engine#55963) 2024-10-18 30870216+gaaclarke@users.noreply.github.com Started filtering out close line segments in rrect polylines. (flutter/engine#55929) 2024-10-18 chinmaygarde@google.com [Impeller] libImpeller: Allow custom font registrations. (flutter/engine#55934) 2024-10-18 chris@bracken.jp Re-reland "iOS: Migrate FlutterEngine to ARC" (flutter/engine#55962) 2024-10-18 chinmaygarde@google.com [Impeller] libImpeller: Add a README. (flutter/engine#55940) 2024-10-18 chris@bracken.jp iOS: Eliminate needless profiler metrics ivar (flutter/engine#55957) 2024-10-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] one descriptor pool per frame. (#55939)" (flutter/engine#55959) 2024-10-18 chris@bracken.jp Revert "Reland "iOS: Migrate FlutterEngine to ARC" (#55937)" (flutter/engine#55954) 2024-10-18 skia-flutter-autoroll@skia.org Roll Dart SDK from 993d3069f42e to a51df90298ca (7 revisions) (flutter/engine#55951) 2024-10-18 jonahwilliams@google.com [engine] add back opt out for merged threads. (flutter/engine#55952) 2024-10-18 jonahwilliams@google.com [Impeller] one descriptor pool per frame. (flutter/engine#55939) 2024-10-18 jonahwilliams@google.com [Impeller] add mechanism for sharing bdf inputs. (flutter/engine#55701) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 9F_NaKPd2twh to tNQZ8d5mRYpe If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jonahwilliams@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

reverted #55939 due to postsubmit failures. It turns out the lifecycle is not exactly the same as the command pool. Instead, use a thread id map on the context to keep the right thread running and simplify shutdown. Leaving CI yaml edit for now to prove it works :) Fixes flutter/flutter#157115

Creating descriptor pools is expensive, no need to have more than one per frame. The lifecycle of the descriptor pool is exactly the same as the cmd pool, which indicates that we can probably do some consolidation/refactoring in the future. Fixes flutter#157115

…lutter/engine#55959) Reverts: flutter/engine#55939 Initiated by: jonahwilliams Reason for reverting: failing on scenario test Original PR Author: jonahwilliams Reviewed By: {matanlurey} This change reverts the following previous change: Creating descriptor pools is expensive, no need to have more than one per frame. The lifecycle of the descriptor pool is exactly the same as the cmd pool, which indicates that we can probably do some consolidation/refactoring in the future. Fixes flutter#157115

reverted flutter/engine#55939 due to postsubmit failures. It turns out the lifecycle is not exactly the same as the command pool. Instead, use a thread id map on the context to keep the right thread running and simplify shutdown. Leaving CI yaml edit for now to prove it works :) Fixes flutter#157115

[Impeller] one descriptor pool per frame.

536007a

github-actions bot added the e: impeller label Oct 17, 2024

add unit test.

6ab4460

flutter deleted a comment from flutter-dashboard bot Oct 18, 2024

jonahwilliams commented Oct 18, 2024

View reviewed changes

jonahwilliams added 4 commits October 17, 2024 17:03

fix comment.

8432312

hook into existing TLS storage.

d973b2c

++

0ae06a8

++

a7398b9

jonahwilliams commented Oct 18, 2024

View reviewed changes

jonahwilliams requested a review from matanlurey October 18, 2024 16:15

matanlurey approved these changes Oct 18, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2024

auto-submit bot merged commit 42c2a40 into flutter:main Oct 18, 2024

jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Oct 18, 2024

auto-submit bot pushed a commit that referenced this pull request Oct 18, 2024

Revert "[Impeller] one descriptor pool per frame. (#55939)"

b6492e4

This reverts commit 42c2a40.

auto-submit bot mentioned this pull request Oct 18, 2024

Reverts "[Impeller] one descriptor pool per frame. (#55939)" #55959

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Oct 18, 2024

jonahwilliams mentioned this pull request Oct 18, 2024

[Impeller] Reland: one descriptor pool per frame. #55960

Merged

engine-flutter-autoroll mentioned this pull request Oct 18, 2024

Roll Flutter Engine from 76d310e42c06 to 6b51fae6c01a (8 revisions) flutter/flutter#157197

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 18, 2024

42c2a40 [Impeller] one descriptor pool per frame. (flutter/engine#55939)

5979d31

engine-flutter-autoroll mentioned this pull request Oct 19, 2024

Roll Flutter Engine from 76d310e42c06 to 0d8a8766cbff (16 revisions) flutter/flutter#157213

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 19, 2024

42c2a40 [Impeller] one descriptor pool per frame. (flutter/engine#55939)

b5ddcb4

engine-flutter-autoroll mentioned this pull request Oct 19, 2024

Roll Flutter Engine from 76d310e42c06 to dd37446fbc55 (17 revisions) flutter/flutter#157215

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 19, 2024

42c2a40 [Impeller] one descriptor pool per frame. (flutter/engine#55939)

c16071e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] one descriptor pool per frame.#55939

[Impeller] one descriptor pool per frame.#55939
auto-submit[bot] merged 6 commits intoflutter:mainfrom
jonahwilliams:one_descriptor_pool_per_frame

jonahwilliams commented Oct 17, 2024

Uh oh!

jonahwilliams Oct 18, 2024

Uh oh!

jonahwilliams Oct 18, 2024

Uh oh!

jonahwilliams Oct 18, 2024

Uh oh!

jonahwilliams Oct 18, 2024

Uh oh!

jonahwilliams commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jonahwilliams commented Oct 17, 2024

Uh oh!

jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants