Memory regression in windows_home_scroll_perf__timeline_summary

[zanderso]

On the PR https://github.com/flutter/flutter/pull/119199, SkiaPerf is reporting a regression in new gen GC count. Usually those are benign, but in this case it seems to be combined with a regression in old gen GC time, and total time spent in GC becoming bimodal, with the high mode about 30% higher than the old value. Dart's GC appears to be keeping up with the new pressure, so there's no increase in frame build times, but I suspect that if we tracked cpu utilization on Windows, it also would have regressed.

cc @gspencergoog @LongCatIsLooong for https://github.com/flutter/flutter/pull/119199 cc @jonahwilliams who added the benchmark.

https://flutter-flutter-perf.skia.org/e/?begin=1681922609&end=1685459911&keys=Xd41633661a3b6de9cb75a0043a2a5733&queries=arch%3Dintel%26branch%3Dmaster%26config%3Ddefault%26device_type%3Dnone%26device_version%3Dnone%26host_type%3Dwin%26sub_result%3D90th_percentile_frame_build_time_millis%26sub_result%3D99th_percentile_frame_build_time_millis%26sub_result%3Daverage_frame_build_time_millis%26sub_result%3Dold_gen_gc_count%26sub_result%3Dworst_frame_build_time_millis%26test%3Dwindows_home_scroll_perf__timeline_summary&queries=arch%3Dintel%26branch%3Dmaster%26config%3Ddefault%26device_type%3Dnone%26device_version%3Dnone%26host_type%3Dwin%26sub_result%3D90th_percentile_frame_build_time_millis%26sub_result%3D99th_percentile_frame_build_time_millis%26sub_result%3Daverage_frame_build_time_millis%26sub_result%3Dold_gen_gc_count%26sub_result%3Dtotal_ui_gc_time%26sub_result%3Dworst_frame_build_time_millis%26test%3Dwindows_home_scroll_perf__timeline_summary&requestType=0&selected=commit%3D35007%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253Dnone%252Cdevice_version%253Dnone%252Chost_type%253Dwin%252Csub_result%253Dnew_gen_gc_count%252Ctest%253Dwindows_home_scroll_perf__timeline_summary%252C&xbaroffset=35007

[LongCatIsLooong]

I don't have a Windows device to repro this, and looking at the code changes nothing stands out, I wonder if it could be the local const variable here?

https://github.com/LongCatIsLooong/flutter/blob/8a0bb989345f5257e7ae77225ec0dd1b878418ec/packages/flutter/lib/src/material/tooltip.dart#L516-L523

[jonahwilliams]

const is a compile time constant, it doesn't matter where you define it.

[LongCatIsLooong]

@jonahwilliams do const variables get gc'ed when they're out of scope? I think I've seen a (potentially) similar regression: https://github.com/flutter/flutter/pull/126382

[jonahwilliams]

I think that you should acquire one of the desktop platforms and verify this locally. If you don't have any please talk to your manager and gcard something, guess and check is unlikely to be productive.

do const variables get gc'ed when they're out of scope?

constants are constant, they don't get allocated or deallocated.

[LongCatIsLooong]

~Ok getting a cloudtop windows~ Update: still waiting for things to get approved. Hopefully the windows cloudtop will be ready on Monday.

[loic-sharma]

@jonahwilliams do const variables get gc'ed when they're out of scope? I think I've seen a (potentially) similar regression: #126382

From my understanding the allocation there wasn't the const list, but rather, the iterable for that const list.

[jonahwilliams]

Ahh yeah, you will create a new iterable. That can be avoided by using a for (var i = 0...) style loop

[zanderso]

From Engine triage. This benchmark is running in a VM https://chromium-swarm.appspot.com/bot?id=flutter-prod-windows-us-central1-a-9-h1mw, so timing info will be suspect and/or noisy. There's an issue open that I'll cross link later for getting machines in the device lab for running host-only tests (and desktop benchmarks like this one.)

It should be an easy fix to change to a regular for-loop from an iterator, though, so I'll still mark this P3 and assign to @LongCatIsLooong.

[LongCatIsLooong]

I couldn't reproduce this on my cloudtop instance, using dart bin/test_runner.dart test -t windows_home_scroll_perf__timeline_summary | grep gc_. The 2 commit hashes I used: #9da1d and #1b800. results:

#9da1d (before):

new_gen_gc_count: 66,
old_gen_gc_count: 6,
total_ui_gc_time: 36.9700000

#1b800 (after):

new_gen_gc_count: 66,
old_gen_gc_count: 9,
total_ui_gc_time: 35.147

@zanderso I think the iterator issue is already fixed. It was tracked by another github issue. Should I close this one?

[zanderso]

Sure, if this is mostly just noise from SkiaPerf, we can just close. We'll get a clearer signal when these benchmarks are running on dedicated machines in the device lab.

[github-actions[bot]]

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 -v and a minimal reproduction of the issue.