Repository navigation
Regression in frame build time in complex_layout_scroll_perf #130339
Description
SkiaPerf is reporting that the 90%-ile and average frame build time regressed on complex_layout_scroll_perf_bad_ios__timeline_summary by about 30%:
The report is on the commit here 3b8f6c4, containing a Skia roll with unrelated commits, and a manual Dart roll flutter-team-archive/engine#43472 with commits here:
https://dart.googlesource.com/sdk.git/+log/2d98d9e27dae..0b07debd5862
Unfortunately, it's a large Dart roll with many VM changes. Marking P0 only for help with identifying any culprits, if possible.
Activity
zanderso commentedon Jul 11, 2023
The flagged framework commit also updated framework package dependencies, but I'm only seeing updates to dev dependencies, and not regular dependencies of the framework, so I wouldn't expect those updates to have affected frame build times.
jason-simmons commentedon Jul 11, 2023
Bisected this to https://dart.googlesource.com/sdk/+/d8fa6fe25522f45b7070278ad152abe4724cbdf2
@lrhn
lrhn commentedon Jul 11, 2023
There are other mentions of the effect of this change on VM optimization.
See dart-lang/sdk#48282 for discussion.
The existing code was wrong, as in the iterator giving completely wrong results in some cases, so it's not just something we can revert.
rakudrama commentedon Jul 11, 2023
For the benchmark to regress 30% on https://dart.googlesource.com/sdk/+/d8fa6fe25522f45b7070278ad152abe4724cbdf2 it must be doing almost nothing except iterating the
.keysor.valuesof an emptyMap.Our worst other regression was a micro-benchmark that did nothing other than iteration of singleton Map's
.keysand was nearer 10%.This seems a little unlikely, so can you validate the finding by profiling before and after to identify where the time is going?
jason-simmons commentedon Jul 12, 2023
I can reliably reproduce the effect of https://dart.googlesource.com/sdk/+/d8fa6fe25522f45b7070278ad152abe4724cbdf2 on the
complex_layout_scroll_perf_bad_ios__timeline_summaryframe build time metrics.I tried capturing some profiles of the app used in the benchmark and noticed that it's spending significant time in
Layer._fireCompositionCallbacks.(https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/rendering/layer.dart#L175)
That function is iterating over a map that is apparently always empty in this app. It consumes a lot of the time that is accounted as "frame build time" in the benchmark.
If I patch
Layer._fireCompositionCallbacksto skip the iteration if the map is empty, thenaverage_frame_build_time_millisis reduced by 25%.@a-siva Adding the
@pragma('vm:prefer-inline')annotation to the_CompactIterableiterator getter did not have a major effect.mraleph commentedon Jul 12, 2023
Comment dart-lang/sdk#48282 (comment) describes the reason for the regression. tldr here is that
_CompactIterable.iteratornow has two additional virtual calls. This would only matter if you are iterating really small maps (~1 element) so the overhead of creating the iterator dominates the rest. I am going to look at fixing this, but in the interim I suggest just skipping the iteration altogether.Also it's interesting that iOS regressed but Android did not?
lrhn commentedon Jul 12, 2023
Maybe the Android version is JIT compiled. I believe Android supports both AoT and JIT, where iOS only supports AoT.
mraleph commentedon Jul 12, 2023
I am pretty sure Flutter only runs benchmarks in profile mode (which corresponds to release AOT in Dart parlance).
Skip the iteration in Layer._fireCompositionCallbacks if the callback…
Skip the iteration in Layer._fireCompositionCallbacks if the callback…
chinmaygarde commentedon Jul 17, 2023
The regression is expected but a small amelioration has landed in #130438. Closing.
github-actions commentedon Jul 31, 2023
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of
flutter doctor -vand a minimal reproduction of the issue.