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

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


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

android-webview-nexus5X
android-webview-nexus6

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

=== Auto-CCing suspected CL author rune@opera.com ===

Hi rune@opera.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 : Rune Lillesveen
  Commit : 9f0c480da9cb188c82e16e6c6751875a60a114cf
  Date   : Wed Sep 27 02:59:59 2017
  Subject: Reland "Store new ComputedStyle object if style did not change."

Bisect Details
  Configuration: android_webview_nexus6_aosp_perf_bisect
  Benchmark    : memory.top_10_mobile
  Metric       : memory:webview:all_processes:reported_by_chrome:cc:effective_size_avg/background/after_https_m_facebook_com_rihanna
  Change       : 19.97% | 16986112.0 -> 20377600.0

Revision             Result                   N
chromium@504552      16986112 +- 0.0          6      good
chromium@504558      16986112 +- 0.0          6      good
chromium@504560      16986112 +- 0.0          6      good
chromium@504561      20377600 +- 0.0          6      bad       <--
chromium@504564      20377600 +- 0.0          6      bad
chromium@504575      20942848 +- 3095991      6      bad
chromium@504598      20377600 +- 0.0          6      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=https.m.facebook.com.rihanna memory.top_10_mobile

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

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


For feedback, file a bug with component Speed>Bisection

Issue 771309 has been merged into this issue.

It's a mystery that this ComputedStyle change affects cc memory and only on Android WebView.

Also only on just two pages. The largest regression is on FB which is about 20% and  2-4 MB.

Just an idea: looking at that CL it seems that it does fix a corner case where hover style was not being properly updated. If so, is it possible that FB page is using hover style in a way that it causes my layers to be created?

Maybe grab a couple of traces, before and after the change, and compare them to see if that helps confirming your theory?

Issue 774084 has been merged into this issue.

So, I've struggled a lot to figure out how to repro. I've not been able to figure out how to have a local setup here using the wprgo file.

What I _have_ done, is to build and run webview_instrumentation_apk with the live https://m.facebook.com/rihanna site and done memory-infra tracing from chrome://inspect?tracing on a Samsung Galaxy S8 which is the only phone I have.

I did not see a difference in cc memory use from that tracing data. Showing ~43MB of cc after initial loading both with and without the CL.

Note you can get traces right from the perf dashboard and the bisect. Here are some from bisect:

at r504560:
https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/after_https_m_facebook_com_rihanna_2017-10-03_13-19-01_79795.html

at r504561:
https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/after_https_m_facebook_com_rihanna_2017-10-03_13-09-13_52065.html 

Thanks sullivan@. That is helpful. Looking at those traces it appears that the extra 3MB of memory used in CC is in cc/resource_memory/provider_1/resources. Before patch there is 5 of those (3MB each) and after patch there is 6 which leads to the increase. I am not sure what can cause resource pool memory increase in cc but ericrk@ may have some ideas.

I don't have access to those traces since I don't have a google account.

Looking at the source code, ResourceProvider is one of:

  RESOURCE_TYPE_GPU_MEMORY_BUFFER,
  RESOURCE_TYPE_GL_TEXTURE,
  RESOURCE_TYPE_BITMAP,

The trace doesn't say which type, I guess.

Adding reviewers nainar and treib to see if they can help

I reverted the first land of this CL because it broke a test, I have no idea what's going on here.

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

I'm about to pack things down here. Note that reverting the change in question will re-introduce a serious issue:  767832 , unless we also revert https://chromium.googlesource.com/chromium/src/+/a87ff17992abb8eea93e035b7e355824f5ff74d9

I'll take on ownership of this as per offline conversation. 

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

Suspected Commit
  Author : Rune Lillesveen
  Commit : 9f0c480da9cb188c82e16e6c6751875a60a114cf
  Date   : Wed Sep 27 02:59:59 2017
  Subject: Reland "Store new ComputedStyle object if style did not change."

Bisect Details
  Configuration: android_webview_arm64_aosp_perf_bisect
  Benchmark    : memory.top_10_mobile
  Metric       : memory:webview:all_processes:reported_by_os:system_memory:native_heap:proportional_resident_size_avg/foreground/http_en_m_wikipedia_org_wiki_Science
  Change       : 0.87% | 48200472.0 -> 48620312.0

Revision             Result                  N
chromium@504406      48200472 +- 305550      6       good
chromium@504484      48221464 +- 463226      6       good
chromium@504523      48100632 +- 273374      6       good
chromium@504543      48131864 +- 395905      6       good
chromium@504553      48274996 +- 380404      9       good
chromium@504558      48384792 +- 805902      14      good
chromium@504560      48437747 +- 771058      14      good
chromium@504561      48778804 +- 416177      9       bad       <--
chromium@504562      48762762 +- 496845      9       bad
chromium@504717      48620312 +- 443308      6       bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=http.en.m.wikipedia.org.wiki.Science memory.top_10_mobile

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

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


For feedback, file a bug with component Speed>Bisection

To add a bit more info - it appears that we have one more CC tile resource after the change. Is it possible that the change impacted css/dom layerization on the rihanna page? If anything caused us to layerize additional content, it could definitely lead to this regression (and is probably safe to ignore in this case).

I dont have the expertise to investigate this issue so I would prefer reverting over anything. I have a CL in CQ

rune@ is now offline. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1bff011ab3fc8f42feb1d27f796cabfc2eef7da

commit a1bff011ab3fc8f42feb1d27f796cabfc2eef7da
Author: Naina Raisinghani <nainar@chromium.org>
Date: Thu Oct 19 00:02:59 2017

Revert rune@'s changes needed to fix perf  issue 771294 

This is a revert of the following 2 issues:
1. No need for forced SetStyleInternal for unchanged ComputedStyle.
2. Reland 'Store new ComputedStyle object if style did not change.

Reverts generated via git revert.
Done on behalf of rune@ who has now left Opera. 

TBR=shend@chromium.org

Bug:  771294 
Change-Id: I450b66124e30978fb731e05a3528395bb8862f3d
Reviewed-on: https://chromium-review.googlesource.com/726719
Commit-Queue: nainar <nainar@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509934}
[delete] https://crrev.com/b82e8b119b85a6c9e364a24624735b2c5c6e39da/third_party/WebKit/LayoutTests/fast/dynamic/hover-after-affected-by-change.html
[modify] https://crrev.com/a1bff011ab3fc8f42feb1d27f796cabfc2eef7da/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/a1bff011ab3fc8f42feb1d27f796cabfc2eef7da/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/a1bff011ab3fc8f42feb1d27f796cabfc2eef7da/third_party/WebKit/Source/core/style/ComputedStyle.h

I can't see that this revert caused any of the graphs to recover.

Correct, the graphs did not recover, e.g. the main regression:
https://chromeperf.appspot.com/report?sid=12d872e3b824e5fd51cb42d5d6cea2e2d6e5d42be931d9ce34308fca704e19a0&start_rev=502074&end_rev=512098

If you zoom in quite a lot there is a ~ 32KiB drop when #24 landed, although the original regression (also reproduced by the bisect) is of about 3.2MiB.

In case they help to investigate, these are a couple of traces around #24:
before: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/after_https_m_facebook_com_rihanna_2017-10-18_19-48-47_13614.html
after: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/after_https_m_facebook_com_rihanna_2017-10-19_02-13-25_13883.html

Note, the benchmark changed between the original landings of r498417 (No need for ...) and r504561 (Reland Store new ...), so this might help to explain why reverting both didn't bring us back to the same baseline?

I'm happy to either keep things as-is or revert #24 while things are further investigated. Either way, we should probably continue the investigation to figure out what's going on.

I am happy to revert my change and continue the investigation, if rune@lillesveen@ sees fit.

futhark: any progress on this bug?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/394e62d9a30231f2d4a226e5e2f585ec36397b81

commit 394e62d9a30231f2d4a226e5e2f585ec36397b81
Author: Rune Lillesveen <futhark@chromium.org>
Date: Fri Jan 19 10:14:49 2018

Use new ComputedStyle instance for kNoChange.

Even if the computed style doesn't change, AffectedBy* flags may have
changed, which means we need to set the current ComputedStyle to the new
instance. We can however skip any invalidation diffs.

This is a re-land of the revert of this functionality which did not make
us recover from the memory regression reported in 771294.

Bug:  768406 ,  771294 
Change-Id: Icc7eeee8a982eed53243d52d04d45e313d79a10d
Reviewed-on: https://chromium-review.googlesource.com/870391
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530468}
[add] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/LayoutTests/external/wpt/css/selectors/hover-002-manual.html
[add] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/LayoutTests/external/wpt_automation/css/selectors/hover-002-manual-automation.js
[modify] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/LayoutTests/resources/testharnessreport.js
[modify] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/394e62d9a30231f2d4a226e5e2f585ec36397b81/third_party/WebKit/Source/core/style/ComputedStyle.h

Based on reverting the CL did not have an effect, re-landing it didn't have an effect, I think we can wontfix this. I guess the big increase (3.2MB) could be due to gpu buffers being allocated at that size and the change itself could be a minor increase causing the need for a new block. The graphs later dropped and then went back up again unrelated to the CL discussed in this report.