Get bounds from RTree in DLBuilder::Build() #50253
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. |
|
I will be seeking a test exemption on the grounds that this change isn't externally testable since it is just a code efficiency change. We already have code that tests the computation of bounds for a DisplayList so this change should be covered by existing tests. |
|
Note that the added keyword in the |
|
No ground-breaking benchmark results to show. I did a quick survey of framework benchmarks that I suspect would see an improvement, but the gains were in the noise. This improvement may not show through the noise in the simple apps we use for benchmarking. |
|
test-exempt: performance improvement |
…142783) flutter/engine@dd4c79a...b35153d 2024-02-02 [email protected] Manual roll Dart SDK from 82936dcdaf4f to 5a5d4c262200 (2 revisions) (flutter/engine#50264) 2024-02-01 [email protected] Remove number of arguments from defining Dart FFI (flutter/engine#50153) 2024-02-01 [email protected] Refactor the linux_android targets to make use of recent changes to android virtual device params (flutter/engine#50099) 2024-02-01 [email protected] Re-add tests deleted on accident (flutter/engine#50223) 2024-02-01 [email protected] [Impeller] updated todos from opengles golden work (flutter/engine#50218) 2024-02-01 [email protected] Get bounds from RTree in DLBuilder::Build() (flutter/engine#50253) 2024-02-01 [email protected] [Impeller] Remove unused define. (flutter/engine#50250) 2024-02-01 [email protected] Add the focus state related methods to the platform dispatcher (flutter/engine#49841) 2024-02-01 [email protected] Fix the output paths of the Web esbuild GN template (flutter/engine#50188) 2024-02-01 [email protected] [Impeller] Delete unnecessary special casing for Vulkan in framebuffer fetch. (flutter/engine#50251) 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://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
This optimization was discovered when reviewing the bounds accumulation code in DisplayListBuilder. There is trivially wasted work in the
Build()method that can be easily avoided.When a Builder is preparing an RTree, it accumulates a list of bounds for every rendering op, but doesn't union them up front. If you ask the Builder for the bounds of the accumulated ops, it will have to union all of the rectangles to produce an answer. But, if you ask the Builder to produce an RTree, that process will implicitly combine all of the rectangles as a side effect of creating the tree structure - and that RTree object has a bounds method that can return the result directly.
This change leverages the construction of the RTree to avoid having to union all of the rects twice which can only save time when constructing the majority of DisplayList objects used in a scene.