[Impeller] Added recycled command buffers. #44904
Conversation
gaaclarke
force-pushed
the
recycle-command-buffers-2
branch
2 times, most recently
from
843427c to
88fe3bc
Compare
|
|
||
| // This number comes from observing the recycled_buffers_ size while running | ||
| // Wonderous and seeing it cap out at 7. | ||
| static constexpr size_t kMaxRecycledBufferSize = 10; |
There was a problem hiding this comment.
This may be moot since they will still probably live in the pool.
There was a problem hiding this comment.
I'm not sure this is necessary since like you mention, its the pool itself that has memory. reseting/freeing should be more or less the same right?
There was a problem hiding this comment.
Yea, kMaxRecycledBufferSize isn't going to really work because it seems like the pool should be holding onto the command buffers anyways. I removed it.
| break; | ||
| } | ||
| buffer->reset(); | ||
| recycled_buffers_.emplace_back(std::move(buffer)); |
There was a problem hiding this comment.
We might be able to get away with calling reset and release, letting the pool handle the recycling.
There was a problem hiding this comment.
I tried that and it didn't seem to be working as I expected. Printing out the CommandBuffers that are created didn't seem to recycle. Actually keeping the CommandBuffer around is a better way to assert that it is recycling at least.
There was a problem hiding this comment.
command buffers themselves don't own any memory, the command pool is where the pooled memory is.
chinmaygarde
changed the title
|
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. |
gaaclarke
force-pushed
the
recycle-command-buffers-2
branch
from
8d69b41 to
9418d73
Compare
…133005) flutter/engine@55b8426...e5929d8 2023-08-21 [email protected] [Impeller] Disable Impeller on Fuchsia (flutter/engine#44925) 2023-08-21 [email protected] [Impeller] Added recycled command buffers. (flutter/engine#44904) 2023-08-21 [email protected] Roll Dart SDK from 1859f7202bba to 00f6f69187df (1 revision) (flutter/engine#44922) 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 [email protected],[email protected],[email protected] 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: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Recycles command buffers instead of deleting them when they are done being used. @jonahwilliams profiled the Gallery on the Pixel 7 pro and saw that sometimes `RenderPassVK::OnEncodeCommands` takes a long time in flutter/flutter#132690. After adding adding more tracing it was found that the culprit was deleting the CommandBuffers on the raster thread. This PR removes the deletion of the CommandBuffers and instead resets and recycles them.  > where GARBO2 is > ```c++ > void CommandPoolVK::GarbageCollectBuffersIfAble() { > TRACE_EVENT0("impeller", "CommandPoolVK::GARBO2"); > if (std::this_thread::get_id() != owner_id_) { > return; > } > buffers_to_collect_.clear(); > } > ``` This approach matches the guidances from ["Vulkan Best Practice for Mobile Developers"](https://arm-software.github.io/vulkan_best_practice_for_mobile_developers/samples/performance/command_buffer_usage/command_buffer_usage_tutorial.html) where it is preferable to recycle command buffers than delete them. Also, the recommendation is that if you are not reusing command buffers that you should periodically reset the pool, which we weren't doing. issue: flutter/flutter#132690 ## 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 Hixie said 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 [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
|
This was a 10% drop in worst frame build time for AVERAGE(62.63, 67.574, 59.244, 59.59, 67.145, 66.214, 55.63, 58.981, 56.337) / AVERAGE(53.647, 53.346, 59.414, 54.581, 55.753, 55.81, 55.447) = 0.9015253981 |
2 participants
Recycles command buffers instead of deleting them when they are done being used.
@jonahwilliams profiled the Gallery on the Pixel 7 pro and saw that sometimes
RenderPassVK::OnEncodeCommandstakes a long time in flutter/flutter#132690. After adding adding more tracing it was found that the culprit was deleting the CommandBuffers on the raster thread. This PR removes the deletion of the CommandBuffers and instead resets and recycles them.This approach matches the guidances from "Vulkan Best Practice for Mobile Developers" where it is preferable to recycle command buffers than delete them. Also, the recommendation is that if you are not reusing command buffers that you should periodically reset the pool, which we weren't doing.
issue: flutter/flutter#132690
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.