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

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


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

chromium-rel-mac12

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8966629920033100672

=== BISECT JOB RESULTS ===
Perf regression found but unable to narrow commit range

Build failures prevented the bisect from narrowing the range further.


Bisect Details
  Configuration: mac_10_12_perf_bisect
  Benchmark    : memory.desktop
  Metric       : memory:chrome:all_processes:reported_by_chrome:cc:effective_size_avg/TrivialBlurAnimationPageSharedPageState
  Change       : 10.52% | 23130965.7143 -> 25565160.0

Suspected Commit Range
  2 commits in range
  https://chromium.googlesource.com/chromium/src/+log/7fa7a4540e6ed9172ed26459f95a4207826253d8..afd24343fb79bb4212e26be973a51f6d9713e556


Revision             Result                   N
chromium@505837      23130966 +- 7887691      14       good
chromium@505899      22826691 +- 7637224      14       good
chromium@505907      23435240 +- 7969431      14       good
chromium@505911      23435240 +- 7969431      14       good
chromium@505912      ---                      ---      build failure
chromium@505913      25869434 +- 4104885      14       bad
chromium@505915      25869434 +- 4104885      14       bad
chromium@505930      25565160 +- 0.0          9        bad
chromium@505960      25565160 +- 0.0          14       bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=TrivialBlurAnimationPageSharedPageState memory.desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8966629920033100672


For feedback, file a bug with component Speed>Bisection

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8966607394314587040

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8966606665607732768

=== BISECT JOB RESULTS ===
Perf regression found but unable to continue

Bisect was stopped because a commit couldn't be classified as either
good or bad.


Bisect Details
  Configuration: mac_10_12_perf_bisect
  Benchmark    : memory.desktop
  Metric       : memory:chrome:all_processes:reported_by_chrome:cc:effective_size_avg/TrivialBlurAnimationPageSharedPageState

Revision             Result                   N
chromium@505907      23536665 +- 9749447      21      good
chromium@505911      23942364 +- 9479825      21      unknown
chromium@505915      25565160 +- 0.0          21      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=TrivialBlurAnimationPageSharedPageState memory.desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8966606665607732768


For feedback, file a bug with component Speed>Bisection

=== Auto-CCing suspected CL author xing.xu@intel.com ===

Hi xing.xu@intel.com, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Xu Xing
  Commit : afd24343fb79bb4212e26be973a51f6d9713e556
  Date   : Tue Oct 03 01:43:55 2017
  Subject: Avoid checking overlay ResourcePool in non CA Layer

Bisect Details
  Configuration: mac_10_12_perf_bisect
  Benchmark    : memory.desktop
  Metric       : memory:chrome:all_processes:reported_by_chrome:cc:effective_size_avg/TrivialBlurAnimationPageSharedPageState
  Change       : 8.62% | 23536664.7619 -> 25565160.0

Revision             Result                   N
chromium@505910      23536665 +- 9749447      21      good
chromium@505912      22725267 +- 4918840      6       good
chromium@505913      26038476 +- 4016216      9       bad       <--
chromium@505915      25565160 +- 0.0          21      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=TrivialBlurAnimationPageSharedPageState memory.desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8966607394314587040


For feedback, file a bug with component Speed>Bisection

piman: Looks like this change had a significant negative impact on desktop memory usage. Maybe revert?

Are we 100% positive? That change modifies a Windows-only path, how does that affect Mac?

Well, technically that function is called on Mac, so it could have an effect, I will revert, but I'm really stumped by this.

Revert here: https://chromium-review.googlesource.com/c/chromium/src/+/703714

Hm. I'm a little bit wary of any bisect result where the noise is > size of regression. e.g. 

 23536665 +- 9749447

seems to imply that there's ~40% on this metric. And there were multiple bisect failures earlier? sullivan: why don't I see pinpoint graphs for this? I think that would make the problem a lot more obvious.

I'm not familiar with viz, but the fact that this logic is hit at all is wary. 

Revert landed, we can see if the numbers move back.

We've only got pinpoint on a few bots, will convert the rest over the next few weeks.

Graph has recovered, I'm going to mark as closed. Still not 100% sure because of noise, but it looks like this was indeed at least part of the effect.

I suspect that the effect comes from the fact that ResourcePool::SetResourceUsageLimits gets called with a more aggressive number: max_resource_count == 0 always in ScheduleDCLayers when that's called on Mac, whereas it could have been higher in ScheduleCALayers depending on how many render passes we have. So in practice today, we're accidentally way more aggressive with releasing resources than we desire, and that CL accidentally fixes this, which causes a memory regression.

Suggestion: You might want to commit a simple one-liner code comment so that future devs don't accidentally fix the "optimization." Or, explain that the code is now intended as a memory optimization for Mac?

Yeah, this originally regressed by https://codereview.chromium.org/2736643004 a few months ago (too long to check the graphs unfortunately). Xing's CL accidentally fixes things. I'm tempted to revert the revert, and we pay for the regression. @erikchen: thoughts?

> So in practice today, we're accidentally way more aggressive with releasing resources than we desire, and that CL accidentally fixes this, which causes a memory regression.

This is a case for a legitimate trade-off between memory usage and performance. I wish we had tests or metrics that verified the appropriate tradeoff, but I bet we don't. e.g. if we're releasing resources too aggressively, shouldn't we see a performance hit *somewhere*?

In general, I don't think the perf/memory teams should be final arbiters of memory usage [we don't understand the tradeoff space as well as code authors]. And I don't think it's a good use of time to create metrics/tests to justify every single small change.  

Can you provide a written description of:
  1) Expected memory usage.
  2) Expected performance gains.

I hope to be able to reply: hm, that sounds reasonable, ship it.

@#16: I don't think we want to leave it in this state either way. The 2 reasonable options are:
1- explicitly make Mac (ScheduleCALayers) call SetResourceUsageLimits with max_resource_count == 0, and remove the logic in ScheduleDCLayers
2- remove the logic in ScheduleDCLayers and let ScheduleCALayers do what it was meant to do until that broke.

#1 would keep the status quo (no memory regression), #2 would restore the original logic as of 8 months ago.

@#18: I asked you because you wrote that logic originally.

If it's just me, I'll do #1 so that I can move on (I'm really just an innocent bystander except for the fact that I reviewed Xing's CL.)

Apparently I wrote this cache. =P

The purpose of the cache is to optimize for sites that animate CSS filter effects [because creating GMBs is non-trivially expensive]. Without this cache, on every frame we need to recreate all the GMBs used by filter effect layers. This has a significant performance impact on a specific performance suite [the name eludes me right now...]. This also explains the memory regression to TrivialBlurAnimationPageSharedPageState, which I wrote specifically to test this logic.

This cache has no effect for sites that don't trigger the logic path, and the resources expire after 3 seconds, so it really only has a memory impact for sites that are continually animating css filter effects, in which case they also have a significant performance impact. 

We should revert the revert. (2)

->erikchen for the honors, the revert of the revert has landed, closing this as WAI.