All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=909055

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=141284d4417ead126d9c7c0193f6a93a8b71ad99c47f94a638c792dc47de5ebc


Bot(s) for this bug's original alert(s):

linux-perf
mac-10_13_laptop_high_end-perf

blink_perf.parser - Benchmark documentation link:
  https://bit.ly/blink-perf-benchmarks

system_health.memory_desktop - Benchmark documentation link:
  https://bit.ly/system-health-benchmarks

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/10222623e40000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/10222623e40000

Reland "[BlinkGenPropertyTrees] Associate each cc::KeyframeModel with ElementId" by flackr@chromium.org
https://chromium.googlesource.com/chromium/src/+/2a64f7b5b81975a81b3d119b13e0d6776f97c850
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: 2.355e+08 → 2.495e+08 (+1.403e+07)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Benchmark documentation link:
  https://bit.ly/system-health-benchmarks

I will try to dig into this tomorrow. Philip, do you know if BGPT would have been enabled on the perf bot?

There's two obvious memory differences, each KeyframeModel now has an ElementId and should only be if BGPT was enabled that we also pass a set of ElementIds through the commit.

BlinkGenPropertyTrees should not be enabled on perf bots yet.

I am not able to reproduce this locally. I tried reverting on tip of tree and comparing the original revisions where this landed, running with 10 iterations and saw no significant difference between r610493 and r610494 or between tip of tree and the revert, but I do see a difference between r610493/r610494 and tip of tree suggesting there is a real regression but it may be after my commit and perhaps rerunning the bisect might help?

Admittedly I am running this on linux but I have run with --force-device-scale-factor=2 to simulate a high dpi scenario which I assume is the case on the high end mac.

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14eb3022140000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/15c5e154140000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14eb3022140000

Reland "[BlinkGenPropertyTrees] Associate each cc::KeyframeModel with ElementId" by flackr@chromium.org
https://chromium.googlesource.com/chromium/src/+/2a64f7b5b81975a81b3d119b13e0d6776f97c850
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: 2.696e+08 → 2.72e+08 (+2.429e+06)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Benchmark documentation link:
  https://bit.ly/system-health-benchmarks

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/15d445d8140000

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/15d445d8140000

Deadline exceeded while waiting for HTTP response from URL: https://chromium.googlesource.com/chromium/src/+log/a1e93a695f76d88fd0eb2ac5cc070b420349a164..8af3013023099c09a9f579d9ce1dbb7b62dc3692?format=JSON

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/1796519c140000

I think the blink_perf.parser regression is unrelated to the patch. I think some inlining changed and pushed the linux bot over a cliff. No other bots are affected for that test. Here's a link showing other bots on that test: https://chromeperf.appspot.com/report?sid=ad0349f6fff1662e6e8421d2b37cd99770db6ac2612fdc5a11b880683358f6e6&start_rev=605204&end_rev=614330

The system_health.memory_desktop regression seems real though. I can reproduce locally with the following:
[checkout head]
[build chrome release]
./tools/perf/run_benchmark system_health.memory_desktop --browser=release --story-filter=multitab.misc.typical24.2018 --results-label=withpatch --reset-results
git checkout 2a64f7b5b81975a81b3d119b13e0d6776f97c850~1
[build chrome release]
./tools/perf/run_benchmark system_health.memory_desktop --browser=release --story-filter=multitab.misc.typical24.2018 --results-label=nopatch

Here's a link showing several clear regressions:
https://chromeperf.appspot.com/report?sid=8579b13dd87504136e8d56de3bc5c6ea15029ba7c44f020973b7f07d5c1b706c

This is very difficult to dig into since it loads a bunch of tabs. I've kicked off a pinpoint to see if this was your patch.

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/12f31cf4140000

I think it's likely that the memory regression is not your change. I'm going to kick off a bunch of pinpoints.

https://bugs.chromium.org/p/chromium/issues/detail?id=901390 is another bug dealing with the same memory regression. I think the system_health.memory_desktop regression is actually  https://crbug.com/901390 

I think pinpoint is down because I'm getting 500 errors trying to start pinpoint jobs on https://chromeperf.appspot.com/report?sid=8579b13dd87504136e8d56de3bc5c6ea15029ba7c44f020973b7f07d5c1b706c.

I think the remaining task for this bug is to just figure out a way to get pinpoint to run, and then ensure that this patch is actually not the cause.

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14ca4f4a140000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/1715cb9c140000

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/12f31cf4140000

The swarming task expired. The bots are likely overloaded, dead, or misconfigured.

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/1715cb9c140000

Reland "[BlinkGenPropertyTrees] Associate each cc::KeyframeModel with ElementId" by flackr@chromium.org
https://chromium.googlesource.com/chromium/src/+/2a64f7b5b81975a81b3d119b13e0d6776f97c850
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: 6.407e+07 → 6.695e+07 (+2.873e+06)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Benchmark documentation link:
  https://bit.ly/system-health-benchmarks

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/15e39adf940000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/15e39adf940000

Only use all animation compositing reasons for BGPT. by flackr@chromium.org
https://chromium.googlesource.com/chromium/src/+/77543001d35112249228be2745ba072106a93409
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: 2.615e+08 → 2.498e+08 (-1.172e+07)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Benchmark documentation link:
  https://bit.ly/system-health-benchmarks

Looks like the second drop in the memory graph was due to making sure compositing reasons for BGPT were only set when BGPT was enabled (from comment #24). Philip, you mentioned before these compositing reasons should not have caused layer promotion but I can't explain why else my patch would have fixed this regression. Closing the bug since the memory regression seems to have recovered as a result.

Lets reopen this for investigation because BGPT should now be enabled by-default on the perf bots and we should see this regression on the bot. We don't understand why this is not visible on the perf bot. The task is to figure out whether BGPT will actually re-regress this, and whether that needs to block or not.

Looking through my commit[1] which fixed[2] the memory regression, it should be a complete no-op when RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled is true. The memory usage looks relatively stable before/after my patch so the only thing I can figure is that BGPT is not enabled on this bot.

As for whether BGPT will regress this metric, it looks like for non-BGPT the cc-side PropertyTreeBuilder does check for animations before creating the relevant property tree nodes[3] so I imagine that BGPT will create additional property tree nodes that we didn't have before but as far as I can tell the number of composited layers should be the same as any animation would cause a composited layer without BGPT[4] as well. Perhaps the increased memory is simply due to lots of extra nodes in the property trees, in which case we will see the same issue until issue 900241 is resolved.

As for how important this is, it should not be causing additional composited layers so the extra memory should be minimal and we have a plan to recover from it. Philip, I'm not sure what else I can do with this bug short of resolving the underlying issue which is tracked separately.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1374055
[2] https://pinpoint-dot-chromeperf.appspot.com/job/15e39adf940000
[3] https://cs.chromium.org/search/?q=file:property_tree_builder.cc+HasPotentiallyRunning&type=cs
[4] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.cc?type=cs&q=reasons%5C+%5C%7C%3D%5C+CompositingReasonsForAnimation+file:compositing_reason_finder%5C.cc&sq=package:chromium&g=0&l=71

I also made a degenerate test case which if you load with a number in the hash argument it will create that many boxes each with a different type of animation (transform, opacity or filter) and then you can observe the memory usage. I can observe a definite increase in overall process memory with BGPT enabled in the task manager - is there a way I can programmatically check memory usage and/or get more details about allocations?

Deleted: animations.html 832 bytes

animations.html 832 bytes View Download

https://chromium.googlesource.com/chromium/src/+/HEAD/docs/memory-infra/ has information about the memory infra profiler. It's a little hard to use at first but I've found it to be very useful.

Attaching a couple memory dumps from loading animations.html#10000 (i.e. creating 10,000 animations):

The image shows a high level summary,
The BGPT renderer native heap has allocated 53-60 extra megabytes.
I'm not sure what falls into the cc category but for both it is only at 0.3MB.

Comparing both to the same page with only 1 animation (e.g. animations.html#1) we get the following native heap PSS:
Non-BGPT: 7,145 KB -> 245,866 KB =~ 24 KB / animated box
BGPT: 7,536 KB -> 307,118 KB =~ 30 KB / animated box

Interestingly, I decided to compare these to the same page except animating all properties on every box (animations-all.html), and I see a similar story:
Non-BGPT grows to 328,460 KB =~ 32 KB / animated box
BGPT grows to 399,523 KB =~ 39 KB / animated box

This leads me to thinking that it's not actually the property node per animation type but a more general BGPT compositing regression.

Deleted: trace_memory-regular.json.gz 572 KB

trace_memory-regular.json.gz 572 KB Download

Deleted: trace_memory-bgpt.json.gz 572 KB

trace_memory-bgpt.json.gz 572 KB Download

Deleted: animations-memory.png 170 KB

	animations-memory.png 170 KB View Download

Deleted: animations-all.html 734 bytes

animations-all.html 734 bytes View Download

I just ran the perf test locally and confirmed that BGPT is not enabled. It looks like only some perf tests run with experimental flags enabled (https://cs.chromium.org/chromium/src/tools/perf/benchmarks/blink_perf.py?type=cs&q=experimental+file:%5Esrc/tools/perf/+package:%5Echromium$&g=0&l=267)

An increase of 6KB / animated box, while unfortunate, seems like not a big deal to me, but when I look at the memory graph:
https://chromeperf.appspot.com/report?sid=24f664d636d4eaf480a93f7b2cab49e13a4886643dd2811d7ad142b09d2a8179&start_rev=614975&end_rev=624761
The regression is ~17MB in cc memory. In comment #27 you suggested this might be property tree nodes, but I think that is too much to be property tree nodes. Could we be creating more render surfaces for animations when we did not previously?

Looking at PaintArtifactCompositor::UpdateRenderSurfaceForEffects, even though all animated elements have Effect nodes we should only create render surfaces if two such nodes in an ancestor chain have opacity != 1.f. However, it's possible that on sites the animated element was not animating opacity, but was not opacity 1 and some ancestor effect node also has non-1 opacity forcing it to now create a render surface.

If this is the cause though, we again need to wait on issue 900241 so that we can stop creating effect nodes.

Do you know if the 17MB should be showing up in the cc category in my memory-infra trace? I'll see if I can cause render surface creation if this results in more memory usage.

Yeah, I think the perf test and memory infra tracing tool are using the same data, so it should show up.