25% regression in web_benchmarks_skwasm:bench_material3_scroll_semantics.skwasm.drawFrameDuration.average

[gaaclarke]

Introduced in #144208

skia perf

cc @eyebrowsoffire

[eyebrowsoffire]

The PR that triggered this change was a engine roll with a number of changes. I ran benchmarks against a number of the intermediate commits in order to further isolate the issue, and it appears to have happened with this Dart SDK roll specifically: flutter/engine#50979

There are a few dart2wasm-specific changes in that roll, but I haven't yet isolated it down to a specific dart commit.

[mkustermann]

The "root cause" of the regression is according to my analysis - dart-lang/sdk@c625797 - which implemented proper dart:developer.

• After that CL landed, the APIs in dart:developer actually do something as supposed to before where they were stubbed out.
• The fact that this shows as 25% regression means that what this benchmark is measuring is to a large extend measurement logic itself (e.g. timline events etc) itself.
• This can be seen in the profiles of before/after quite clearly: Timeline.startSync / Timeline.finishSync are high on the profile (e.g. called by PipelineOwner.flushSemantics)
• When running the benchmark manually in the browser window, it jitters around, but doesn't actually sroll down and up
• Given the above, I assume the flutter benchmarking system is benchmarking --profile mode and not --release mode?
• Together this somewhat puts into question the usefulness of this benchmark.

In any case, I'll close this as working as intended.

[gaaclarke]

The fact that this shows as 25% regression means that what this benchmark is measuring is to a large extend measurement logic itself (e.g. timline events etc) itself.
Given the above, I assume the flutter benchmarking system is benchmarking --profile mode and not --release mode?

I wouldn't dismiss a benchmark because it is measuring a large portion of profiling overhead. That is the point of --profile mode, to be profiled =) If that mode creates so much overhead that the profiles are no longer useful, that's a problem. Would it be prudent to revisit the performance of dart:developer on the web?

[eyebrowsoffire]

It's not that the profiles are not useful, the baselines have just changed. I think that's perfectly reasonable. I think it's a good idea to just rebaseline this and triage the skia perf notification and close this bug. I just wanted to make sure we weren't actually regressing rendering performance before closing it.

[mkustermann]

I wouldn't dismiss a benchmark because it is measuring a large portion of profiling overhead.

If the program I want to measure behaves - when measuring it - 25% differently from real world, then that's a problem. The numbers just won't be representative of the real app (and we only care about the real app). When profiling & optimizing real world apps one needs to get metrics for the real app.

If that mode creates so much overhead that the profiles are no longer useful, that's a problem. Would it be prudent to revisit the performance of dart:developer on the web?

The problem may not be the dart:developer implementation per se, it just forwards calls to the browser APIs. It may be more due to the framework doing significantly more work in --profile mode than in --release mode. Could it be that we issue too many timeline events (given the work that happens in-between)?

Two more things

• the actual benchmark in the web page doesn't seem to scroll properly (try running it) it just moves few pixels down & up, instead of scrolling new widgets in. Is this intentional?
• the fact that the profile ran just fine before the CL, means that those timeline events aren't needed for the benchmark to run. Can those benchmarks run in --release mode instead?

[gaaclarke]

Could it be that we issue too many timeline events (given the work that happens in-between)?

Maybe that's something we could help with? Elide timeline events that happen at too high of a frequency? I imagine what is an acceptable frequency will depend on the target platform which will be difficult of users to get right in all cases.

[github-actions]

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.