New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 675533 link

Starred by 0 users

Issue metadata

Status: WontFix
Owner:
NOT IN USE
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.1%-35.6% regression in system_health.memory_mobile at 439070:439094

Project Member Reported by jasontiller@chromium.org, Dec 19 2016

Issue description

See the link to graphs below.
 
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Dec 19 2016

Cc: r...@opera.com
Owner: r...@opera.com

=== PERF REGRESSION ===


=== 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 =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Reland: Collect active stylesheets and and apply asynchronously.
Author  : rune
Commit description:
  
This CL enables asynchronously updating the lists of active stylesheets
applying any style changes using rule set invalidations. This means we
more often avoid full style recalcs when we add or remove stylesheets
from the document as well as when the evaluation of media queries
change.

In general, we now alway compare new and old stylesheets by comparing
their rulesets and schedule style invalidations for removed and added
rulesets.

When media queries changes, we used to give completely in and
recalculate all style once we discovered a media query changed its
evaluation. With this patch, we clear rule sets for sheets which
contain media queries which means we will invalidate rules for the sets
before and after the query change. This can be further refined by only
clearing rule sets when the sheets has a media query which actually did
change evaluation, and also just schedule invalidations for rules which
are inside @media rules.

TreeScopeStyleSheetCollectionTest.cpp is removed as it is replaced by
ActiveStyleSheetsTest.cpp which landed earlier.

updateActiveStyle() has been added a few places where
ensureStyleResolver() previously caused active stylesheets to be up-to-
date. ensureStyleResolver() is now merely a method which creates the
StyleResolver if necessary and returns it.

There are some cleanups and code removal which needs to be done after
this CL, but I have left those out to make this CL as small as
possible. For instance resolverChanged(), which synchronously updated
the active stylesheets, has an empty implementation instead of
including a lot of removals in this CL. The code for lazy-appending
stylesheets in StyleResolver is still there, but not in use.

R=meade@chromium.org
BUG= 567021 

Committed: https://crrev.com/9fb5b60edfb769134733009f9447bad3eaf347b0
Review-Url: https://codereview.chromium.org/2557533005
Cr-Original-Commit-Position: refs/heads/master@{#438148}
Cr-Commit-Position: refs/heads/master@{#439092}
Commit  : 90d4ea3d543f0031769b3aacac2d3e084b95fb7d
Date    : Fri Dec 16 12:25:03 2016


===== TESTED REVISIONS =====
Revision         Mean      Std Dev  N  Good?
chromium@439069  14023851  74215.9  6  good
chromium@439082  14043136  49449.8  6  good
chromium@439088  14036139  53374.2  6  good
chromium@439091  14067883  140085   6  good
chromium@439092  19308373  141656   6  bad    <--
chromium@439093  19257856  201313   6  bad
chromium@439094  19269803  115266   6  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 675533

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.tools.maps system_health.memory_mobile
Test Metric: memory:chrome:all_processes:reported_by_os:system_memory:java_heap:proportional_resident_size_avg/load_tools/load_tools_maps
Relative Change: 37.41%

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2837
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8992862572356136864


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5213930744446976

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!

Comment 4 by r...@opera.com, Dec 19 2016

Status: Assigned (was: Untriaged)

Comment 5 by r...@opera.com, Dec 20 2016

Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 20 2016

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

commit a865f6c19c00f0ba9e021c4871b53d2e115a2b00
Author: rune <rune@opera.com>
Date: Tue Dec 20 23:52:11 2016

Clear CSSGlobalRuleSet on StyleEngine::didDetach().

This could free up memory sooner. Found while investigating 675533, but
not confirmed that this fixes that issue.

BUG= 675533 

Review-Url: https://codereview.chromium.org/2589243002
Cr-Commit-Position: refs/heads/master@{#439935}

[modify] https://crrev.com/a865f6c19c00f0ba9e021c4871b53d2e115a2b00/third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp
[modify] https://crrev.com/a865f6c19c00f0ba9e021c4871b53d2e115a2b00/third_party/WebKit/Source/core/css/CSSGlobalRuleSet.h
[modify] https://crrev.com/a865f6c19c00f0ba9e021c4871b53d2e115a2b00/third_party/WebKit/Source/core/dom/StyleEngine.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 21 2016

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

commit b0f03812c4f8ca50a0cc07caaca0c8eafbb2862a
Author: rune <rune@opera.com>
Date: Wed Dec 21 01:55:16 2016

Clear active tree scopes on StyleEngine::didDetach().

clearResolver() is not only called on didDetach(). Make it private and
name it clearResolvers to reflect that it clears scoped resolvers as
well. The comments related to style resolver reconstructruction is
removed as that does not happen anymore.

Clearing m_treeBoundaryCrossingScopes is moved into didDetach()
which was a more natural place.

Clear active and dirty tree scopes in didDetach to not unnecessarily
hang on to any memory associated with them.

These changes were done investigating  issue 675533 , but won't
necessarily fix anything for that issue.

BUG= 567021 , 675533 

Review-Url: https://codereview.chromium.org/2593643002
Cr-Commit-Position: refs/heads/master@{#439973}

[modify] https://crrev.com/b0f03812c4f8ca50a0cc07caaca0c8eafbb2862a/third_party/WebKit/Source/core/dom/StyleEngine.cpp
[modify] https://crrev.com/b0f03812c4f8ca50a0cc07caaca0c8eafbb2862a/third_party/WebKit/Source/core/dom/StyleEngine.h

rune: Any more work planned on this bug, or should we close?

Comment 9 by r...@opera.com, Aug 16 2017

Status: WontFix (was: Started)
I was not able to figure out how to reproduce and I never understood how my change could affect the java heap size.

Sign in to add a comment