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

Conversation

jonahwilliams
Copy link
Contributor

Following some thresholds from ANGLE, lets try allocating fewer of our resources into dedicated memory to see if that improves allocator performance.

flutter/flutter#129737

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.


// Maximum size to use VMA image suballocation. Any allocation greater than or
// equal to this value will use a dedicated VkDeviceMemory.
constexpr size_t kImageSizeThresholdForDedicatedMemoryAllocation =
Copy link
Member

Choose a reason for hiding this comment

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

Let's put these in a more discoverable place. I bet we will add more for buffers and all sort of other tunables. Perhaps //impeller/renderer/backend/vulkan/limits_vk.h? In a later patch is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Sorry about the late review. I thought I lgtmed but I didn't hit submit.

Screenshot 2023-06-29 at 8 16 19 AM

@chinmaygarde chinmaygarde changed the title [Impeller] allocate fewer textures in dedicated memory. [Impeller] Allocate fewer textures in dedicated memory. Jun 29, 2023
jonahwilliams added 2 commits June 29, 2023 09:08
@jonahwilliams
Copy link
Contributor Author

Holding off on this for a bit.

@jonahwilliams jonahwilliams added the Work in progress (WIP) Not ready (yet) for review! label Jun 29, 2023
// See https://github.com/flutter/flutter/issues/128556 . Some devices do
// not have support for coherent host memory so we don't request it here.
return VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT;
return VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The memory we want actually has all three flags here.

flags |= {};
}
} else {
flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_RANDOM_BIT;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The random access bit was the one causing problems. We shouldn't need this since the memory is coherent for reading in the GPU and we're not using it to readback data on the CPU

@jonahwilliams jonahwilliams changed the title [Impeller] Allocate fewer textures in dedicated memory. [Impeller] Allocate fewer textures in dedicated memory and adjust buffer flags. Jun 30, 2023
@jonahwilliams jonahwilliams removed the Work in progress (WIP) Not ready (yet) for review! label Jun 30, 2023
@jonahwilliams
Copy link
Contributor Author

Rebased this and combined it with a cleanup of the buffer memory flags

@jonahwilliams
Copy link
Contributor Author

Friendly ping @chinmaygarde

@jonahwilliams jonahwilliams requested a review from dnfield July 5, 2023 19:59
@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 5, 2023
@auto-submit auto-submit bot merged commit a6efe7b into flutter:main Jul 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 6, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 6, 2023
…sions) (#130097)

Manual roll Flutter Engine from bd2e42b203e1 to 06c936205d96 (27 revisions)

Manual roll requested by zra@google.com

flutter/engine@bd2e42b...06c9362

2023-07-06 dnfield@google.com Handle nested display list clips in Impeller dispatcher (flutter/engine#43442)
2023-07-06 louiseh0313@gmail.com Add Look Up to iOS selection controls (flutter/engine#43308)
2023-07-06 skia-flutter-autoroll@skia.org Roll Skia from 6aaef9cbc3af to 10a43e57e0a6 (1 revision) (flutter/engine#43444)
2023-07-06 skia-flutter-autoroll@skia.org Roll Skia from 620de5ac9f6b to 6aaef9cbc3af (1 revision) (flutter/engine#43441)
2023-07-06 ychris@google.com Revert "[iOS][Keyboard] Wait vsync on UI thread and update viewport inset to avoid jitter." (flutter/engine#43422)
2023-07-06 skia-flutter-autoroll@skia.org Roll ICU from a2961dc659b4 to e8c3bc9ea97d (7 revisions) (flutter/engine#43440)
2023-07-06 zanderso@users.noreply.github.com Further shard clang-tidy runs (flutter/engine#43428)
2023-07-06 dnfield@google.com Release log capture at end of test (flutter/engine#43429)
2023-07-06 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from uGhka7LBG80dM_I3a... to 3D0ft09qP4-Hp_3mQ... (flutter/engine#43439)
2023-07-06 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from amH2MWTworhTLdMWy... to MEXi9Xyj7u24PSx_J... (flutter/engine#43438)
2023-07-06 109111084+yaakovschectman@users.noreply.github.com Use Windows Display Language (flutter/engine#43341)
2023-07-06 skia-flutter-autoroll@skia.org Roll Skia from b2ba6e1d8c0e to 620de5ac9f6b (2 revisions) (flutter/engine#43437)
2023-07-06 skia-flutter-autoroll@skia.org Roll Skia from 50e866b51a64 to b2ba6e1d8c0e (2 revisions) (flutter/engine#43436)
2023-07-06 skia-flutter-autoroll@skia.org Roll Skia from 8ed969b60e98 to 50e866b51a64 (1 revision) (flutter/engine#43435)
2023-07-06 737941+loic-sharma@users.noreply.github.com Fix typo in painting.dart (flutter/engine#43378)
2023-07-06 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from C3Q7MJBYkiin8zw-f... to uGhka7LBG80dM_I3a... (flutter/engine#43433)
2023-07-06 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from iwgWLB4KaXslnaGwK... to amH2MWTworhTLdMWy... (flutter/engine#43432)
2023-07-05 skia-flutter-autoroll@skia.org Roll Skia from a39421eb8d7b to 8ed969b60e98 (6 revisions) (flutter/engine#43431)
2023-07-05 jonahwilliams@google.com [Impeller] Allocate fewer textures in dedicated memory and adjust buffer flags. (flutter/engine#43313)
2023-07-05 skia-flutter-autoroll@skia.org Roll Skia from d6e941012b87 to a39421eb8d7b (4 revisions) (flutter/engine#43427)
2023-07-05 skia-flutter-autoroll@skia.org Roll Skia from e0b9e047332d to d6e941012b87 (2 revisions) (flutter/engine#43424)
2023-07-05 ychris@google.com [iOS] Avoid crash when backdrop filter is null for PlatformViews (flutter/engine#43150)
2023-07-05 skia-flutter-autoroll@skia.org Roll Skia from 401c397f7235 to e0b9e047332d (1 revision) (flutter/engine#43420)
2023-07-05 737941+loic-sharma@users.noreply.github.com [Windows] Fix `FlutterWindow::GetNativeViewAccessible` crash (flutter/engine#43368)
2023-07-05 linxunfeng@yeah.net [iOS] Fix FlutterViewController retain cycle (flutter/engine#43379)
2023-07-05 skia-flutter-autoroll@skia.org Roll Skia from 23052eb28808 to 401c397f7235 (1 revision) (flutter/engine#43419)
2023-07-05 skia-flutter-autoroll@skia.org Roll Skia from d58324bf653d to 23052eb28808 (3 revisions) (flutter/engine#43418)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from iwgWLB4KaXsl to MEXi9Xyj7u24
  fuchsia/sdk/core/mac-amd64 from C3Q7MJBYkiin to 3D0ft09qP4-H

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 aaclarke@google.com,rmistry@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
...
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
…fer flags. (flutter#43313)

Following some thresholds from ANGLE, lets try allocating fewer of our resources into dedicated memory to see if that improves allocator performance.

flutter/flutter#129737
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller needs tests
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants