Cull the RTree bounds when they are forwarded in DrawDisplayList #45358
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 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. |
|
It would be hard to write a unit test to verify this fix because the end result of the problem was not visible - it was a lot of wasted effort under the covers. In particular, the code spent a large amount of time consolidating rectangles that were off screen only to have the code that iterated the rectangles clip them out anyway - resulting in no change to the final RTrees. |
|
If it's just a performance improving change then I think the benchmarks you added should be sufficient. |
|
Yeah, this is tested in the framework by the benchmark added in flutter/flutter#133434, right? |
|
Yes and no. The benchmarks are currently only running on configurations that use code paths that do not go through this particular line of code. They test/track rtree culling in general, but they don't end up running into this line of code. But, when I land this I can add a new variant of the benchmark that does run on a code path that goes through this code to check for regressions. I can't add that until this lands because the benchmark will error out until this perf issue is fixed. (Since it would be landed with bring_up=true, it could be landed without "breaking" the tree, but it takes 5-10 minutes to annoy the test apparatus enough to have it stop trying to run the benchmark and then it retries it just chewing on the bad data over and over for no useful purpose and wasting time on our CI machines.) So, yes, a benchmark variant I plan to land soon after this rolls in will test for this failure. It's just not in this PR itself. |
…sions) (#133924) Manual roll requested by [email protected] flutter/engine@489c399...e496eec 2023-09-02 [email protected] Roll Skia from 2d8849f9f0cc to 15f77147a3ec (1 revision) (flutter/engine#45414) 2023-09-02 [email protected] Roll Fuchsia Mac SDK from OF4TS05qlWCjukWw6... to MesZPNdj-uw8VdCyV... (flutter/engine#45413) 2023-09-02 [email protected] Remove --disable-service-auth-codes (flutter/engine#45356) 2023-09-02 [email protected] [Impeller] Import cstring for memcpy. (flutter/engine#45408) 2023-09-02 [email protected] Roll Dart SDK from cdf1ce0c6d7e to a5c7102af509 (1 revision) (flutter/engine#45412) 2023-09-02 [email protected] Roll ANGLE from 179bd7762ffa to ebf1e7163216 (1 revision) (flutter/engine#45411) 2023-09-02 [email protected] Remove deprecated MOCK_METHODx calls (flutter/engine#45307) 2023-09-02 [email protected] [Impeller] Better demonstrate blur and draw picture? (flutter/engine#45388) 2023-09-02 [email protected] [Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. (flutter/engine#45393) 2023-09-02 [email protected] Roll ANGLE from 962fdf7b7882 to 179bd7762ffa (1 revision) (flutter/engine#45409) 2023-09-02 [email protected] Cull the RTree bounds when they are forwarded in DrawDisplayList (flutter/engine#45358) 2023-09-02 [email protected] Roll Skia from fedff79a6afc to 2d8849f9f0cc (3 revisions) (flutter/engine#45407) 2023-09-02 [email protected] [impeller] premultiply vertices colors. (flutter/engine#45406) 2023-09-01 [email protected] Roll ANGLE from 6a09e41ce6ea to 962fdf7b7882 (224 revisions) (flutter/engine#45400) 2023-09-01 [email protected] Roll Skia from 22ae23891e8e to fedff79a6afc (1 revision) (flutter/engine#45405) 2023-09-01 [email protected] [Impeller] turned on validations for all debug builds (flutter/engine#45350) 2023-09-01 [email protected] Roll Fuchsia Mac SDK from sk7JBGzW1Jw10Wy-T... to OF4TS05qlWCjukWw6... (flutter/engine#45403) 2023-09-01 [email protected] Roll Skia from 2c0405489966 to 22ae23891e8e (1 revision) (flutter/engine#45402) 2023-09-01 [email protected] [Windows] Update vsync on raster thread (flutter/engine#45310) 2023-09-01 [email protected] Roll Dart SDK from a2ea759c16cc to cdf1ce0c6d7e (1 revision) (flutter/engine#45397) 2023-09-01 [email protected] Roll Skia from f3f6c733c7e6 to 2c0405489966 (1 revision) (flutter/engine#45396) 2023-09-01 [email protected] Roll Skia from 02fa14799c6c to f3f6c733c7e6 (1 revision) (flutter/engine#45394) 2023-09-01 [email protected] Roll Skia from d5d3b0d4ee77 to 02fa14799c6c (2 revisions) (flutter/engine#45392) 2023-09-01 [email protected] [ios][ios17][text_input]fix text input system highlight in iOS 17 Beta 7 with firstRectForRange (flutter/engine#45303) 2023-09-01 [email protected] Roll Skia from d6266ef14a7e to d5d3b0d4ee77 (2 revisions) (flutter/engine#45389) 2023-09-01 [email protected] Roll Dart SDK from 0c121a6431cc to a2ea759c16cc (1 revision) (flutter/engine#45384) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from sk7JBGzW1Jw1 to MesZPNdj-uw8 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
Fixes flutter/flutter#133437
With this fix, the benchmarks for pictures with huge bounds become runnable under more conditions. Currently those benchmarks only run on platforms where we don't run into this issue. The performance on those platforms/conditions will not change much, but with this fix we can run them on Impeller and add a variant that has a platform view on the display.