Benchmark complex_layout_scroll_perf__memory needs better metrics to avoid reporting phantom regressions

The main issue (after some discussion) is that this benchmark is not reporting metrics that exactly match its concern and so situations such as those described below can cause it to indicate a regression when a change simply affects its underlying assumptions instead. We need to develop better metrics for it to measure so that it only flags when we have an actual memory regression.

Until then the benchmark acts mostly as a "canary" that will often see a change that might negatively affect app performance, but sometimes in cases that don't really impact real-world performance. Changes in the benchmark should be noted, but more investigation will determine if the change in reported metrics really represents a regression.

Originally reported issue:

I'm investigating the historic slowdowns in the complex_layout_scroll_perf__memory benchmark. It looks like we were at around 5000 in late July and since then we've been stuck around 9000+.

It appears that commit 9b150f1 is responsible for that change. Here are 3 runs of the benchmark on a Moto G4 first running on commit 0d0af31 (which is just before the indicated commit) and then on commit 9b150f1

Stats for hash 0d0af3

On G4:

  "data": {
    "start-min": 34210,
    "start-max": 40838,
    "start-median": 39220,
    "end-min": 44052,
    "end-max": 46190,
    "end-median": 45034,
    "diff-min": 4464,
    "diff-max": 11980,
    "diff-median": 5033
  },
  "data": {
    "start-min": 35164,
    "start-max": 41233,
    "start-median": 40473,
    "end-min": 44973,
    "end-max": 45653,
    "end-median": 45321,
    "diff-min": 4125,
    "diff-max": 10322,
    "diff-median": 4487
  },
  "data": {
    "start-min": 32907,
    "start-max": 40356,
    "start-median": 39693,
    "end-min": 44393,
    "end-max": 48066,
    "end-median": 44879,
    "diff-min": 4099,
    "diff-max": 15159,
    "diff-median": 4907
  },

Stats for hash 9b150f:

On G4:

  "data": {
    "start-min": 31106,
    "start-max": 33471,
    "start-median": 31184,
    "end-min": 40494,
    "end-max": 45019,
    "end-median": 40980,
    "diff-min": 7793,
    "diff-max": 11548,
    "diff-median": 9319
  },
  "data": {
    "start-min": 31144,
    "start-max": 32886,
    "start-median": 31240,
    "end-min": 40506,
    "end-max": 41104,
    "end-median": 40906,
    "diff-min": 7932,
    "diff-max": 9798,
    "diff-median": 9690
  },
  "data": {
    "start-min": 30958,
    "start-max": 32838,
    "start-median": 31365,
    "end-min": 40332,
    "end-max": 41224,
    "end-median": 40892,
    "diff-min": 8054,
    "diff-max": 9874,
    "diff-median": 9501
  },

For the record, I ran the benchmark 3 times on a recent commit (72888c7) and then I reverted 9b150f1 and ran them again. We see significant improvement in the benchmark by reverting.

Note that the revert of 9b150f was clean, but there are 2 new uses of the drawCallSpacing parameter that were added since that commit that need to be manually changed to 80.0 which is what we used to use for that property.

Stats for hash 72888c7:

On G4:

  "data": {
    "start-min": 31382,
    "start-max": 34911,
    "start-median": 31504,
    "end-min": 39259,
    "end-max": 46212,
    "end-median": 40758,
    "diff-min": 7702,
    "diff-max": 11301,
    "diff-median": 9095
  },
  "data": {
    "start-min": 31425,
    "start-max": 32905,
    "start-median": 31506,
    "end-min": 39711,
    "end-max": 42231,
    "end-median": 40842,
    "diff-min": 8086,
    "diff-max": 9831,
    "diff-median": 8850
  },
  "data": {
    "start-min": 29615,
    "start-max": 30938,
    "start-median": 29795,
    "end-min": 37121,
    "end-max": 39081,
    "end-median": 38783,
    "diff-min": 7226,
    "diff-max": 9286,
    "diff-median": 8847
  },

Stats for hash 72888c7 reverting 9b150f:

On G4:

  "data": {
    "start-min": 34044,
    "start-max": 37161,
    "start-median": 36504,
    "end-min": 39901,
    "end-max": 44775,
    "end-median": 40613,
    "diff-min": 2808,
    "diff-max": 10731,
    "diff-median": 4019
  },
  "data": {
    "start-min": 36600,
    "start-max": 37377,
    "start-median": 36669,
    "end-min": 39845,
    "end-max": 41171,
    "end-median": 40771,
    "diff-min": 2952,
    "diff-max": 4357,
    "diff-median": 3683
  },
  "data": {
    "start-min": 36407,
    "start-max": 37249,
    "start-median": 36579,
    "end-min": 40453,
    "end-max": 41077,
    "end-median": 40762,
    "diff-min": 3204,
    "diff-max": 4405,
    "diff-median": 3886
  },

See #36079 (comment)

This should be an improvement instead of a regression: both the start memory and the end memory drops after that PR. The start memory drops more significantly so the diff (end memory - start memory) increases. Presumably, less start memory leaves GC more room to retain more objects so the diff increases. But since the overall memory strictly decreases, this has no negative effect for any users.

That said, one way of fixing this is by fixing #37318

What value is intending to be measured here then? If the benchmark's reported number can go up and down and that doesn't directly translate into a condition that we consider good or bad then the benchmark isn't useful.

Depending on what condition we want to track, a call to GC may or may not be an answer.

If we are concerned with how much garbage is produced by the act of scrolling then measuring the size of the heap before and after doesn't produce a useful value because one run could trigger GC several times and another run might not trigger it at all, yet they'll report similar numbers. In this case, calling GC at the start doesn't really make that number any more useful. We need information from the collector as to how much memory it reclaimed to add to the heap size.

If we are measuring how much RAM is needed to support an app that does scrolling then the more important metric to measure would be the max heap size, presumably the "end" size if the runtime doesn't ever give memory back as a result of GC.

What information are we seeking to gain from this benchmark?

It seems this benchmark is reporting a number that only loosely coordinates with a condition that we are concerned about. Should this benchmark be taken off-line until we can find a more reliable way to measure memory usage?

I agree that this benchmark is not perfect. But we may not want to remove it entirely until we find another better one to replace this benchmark.

I think this benchmarks was not trying to measure the garbage generated. It was trying to capture the least amount of the additional memory to achieve the end state from the start state. So this is more similar to your second interpretation, but instead of never calling a GC (or giving memory back), we want it to call GC as frequently as possible, and always reclaim unnecessary memory. Therefore, this benchmark won't be sensitive to how long we scroll, but only to how efficient we can represent the end state in memory (on top of the start state).

Issue #37318 suggests that we need a GC at the end (NOT the start) state (before taking the measurement) so the end state memory won't include a bunch of uncollected garbage.

Of course, never calling a GC would be a valid benchmark. But it'll depend on how many frames are generated, and has the following unwanted properties:

We'll get out-of-memory crash for longer driver tests
If we suddenly rendered more frames by fixing some jank, the memory benchmark will suddenly go up.

Do we want to close this issue? Maybe open one that is more apropos?

@liyuqian

If we close this, then we should probably consider rebasing the benchmark so it isn't red all the time.

I'll add to the issues that @liyuqian brought up that I think a number of memory statistics would be interesting, and I'm not sure that this benchmark effectively measures any or most of them:

Memory used by dart code
Memory used by C++/engine code
Same memory stats with and without taking GC into account (i.e. how much allocation we do vs. how much we need at any given state)
VRAM used
Minimum memory required (for running on a limited system) for it to run at all (and occasionally running GC doesn't exactly measure that since it could use more memory temporarily and GC would just cut it back to what is in use at the time GC is run).
RAM growth "in the wild" - that is, without any specific back-pressure like running GC artificially while it is doing work (which wouldn't be done for an app in the wild).

@flar

@flar : I thought the benchmark has already been rebased several days after @gaaclarke 's original PR? I didn't see any red lines in complex_layout_scroll_perf__memory. But I was told that our dashboard is having a new backend and something is not working (e.g., the history/details panel). So that might be a dashboard bug? If you still see red lines, please file a bug against the infra and probably attach screenshot there for triaging.

I've closed the "revert" PR associated with this issue and the benchmark has been rebased so it is no longer red. Do we leave this open as relating to "we need to find better metrics to measure with that benchmark"? Or is there another issue related to that?

Sure, let's leave this open and change the issue title to something like "Need better memory metrics".

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.

Navigation Menu

Repository navigation

Benchmark complex_layout_scroll_perf__memory needs better metrics to avoid reporting phantom regressions #40406

10 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Benchmark complex_layout_scroll_perf__memory needs better metrics to avoid reporting phantom regressions #40406

Description

Activity

flar commented on Sep 13, 2019

liyuqian commented on Sep 13, 2019

flar commented on Sep 13, 2019

flar commented on Sep 18, 2019

liyuqian commented on Sep 18, 2019

gaaclarke commented on Sep 24, 2019

flar commented on Sep 24, 2019

liyuqian commented on Sep 24, 2019

flar commented on Sep 25, 2019

liyuqian commented on Sep 25, 2019

10 remaining items

github-actions commented on Mar 13, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions