[Impeller] allow for one more active swapchain image. #52505
Conversation
|
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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. |
chinmaygarde
left a comment
There was a problem hiding this comment.
Just the kMaxPendingPresents should get rid of the wait. See the comment on discord for why this was added. It only handles a runaway situation on the emulators where there were too many pending presents without a completion callback. Ideally we should never see a wait. That we are means we can bump up this number.
| //------------------------------------------------------------------------------ | ||
| /// The maximum number of presents pending in the compositor after which the | ||
| /// acquire calls will block. | ||
| /// acquire calls will block. This value is 2 images given to the system |
There was a problem hiding this comment.
The value is not 2 though.
| std::weak_ptr<Context> context, | ||
| android::HardwareBufferDescriptor desc, | ||
| size_t max_entries = 2u, | ||
| size_t max_entries = 3u, |
There was a problem hiding this comment.
This count is the number of textures idling and used neither by the client (us) or the compositor. I don't think holding onto an additional texture matters here.
There was a problem hiding this comment.
Removed. I think I must be seeing the case where the texture ages quicker than expected.
flutter/engine@bfc6787...fc28057 2024-05-02 jonahwilliams@google.com [Impeller] allow for one more active swapchain image. (flutter/engine#52505) 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://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
|
Here's the drop for this PR. It's about a 15% reduction in |
Currently the rendering performance is less stable with this patch: https://flutter-flutter-perf.skia.org/e/?queries=device_type%3DPixel_7_Pro%26sub_result%3D90th_percentile_frame_rasterizer_time_millis%26sub_result%3D99th_percentile_frame_rasterizer_time_millis%26sub_result%3Daverage_frame_rasterizer_time_millis%26sub_result%3Dworst_frame_rasterizer_time_millis%26test%3Dcomplex_layout_scroll_perf_impeller__timeline_summary&selected=commit%3D40592%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DPixel_7_Pro%252Cdevice_version%253Dnone%252Chost_type%253Dlinux%252Csub_result%253D99th_percentile_frame_rasterizer_time_millis%252Ctest%253Dcomplex_layout_scroll_perf_impeller__timeline_summary%252C
The reason I suspect that is that we're signaling the semaphore at the start of the raster thread. This means that we're limiting the actual number of pending presents to 1, as we also need to account for the CPU access. Increase this and the max texture count to 3, to allow 2 pending presents and one image worked on by the CPU.