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

Issue 635922 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
NOT IN USE
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

2.2% regression in system_health.memory_desktop at 410410:410509

Project Member Reported by petrcermak@chromium.org, Aug 9 2016

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=635922

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgtvmgsgoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgtszkowkM


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

chromium-rel-win7-x64-dual
chromium-rel-win8-dual
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Aug 10 2016

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

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

Hi rune@opera.com, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Style invalidation support for :first/last/only-child.
Author  : rune
Commit description:
  
Got rid of SubtreeStyleChanges for those pseudo classes. Also fixed
issue 245914 by looking at next/previous element, not node, to figure
out if we are the first. The code in checkForSiblingStyleChanges could
be restructured quite a bit and made simpler now have changedElement
(changedNode => changeElement since the input is always an element).

BUG=245914

Review-Url: https://codereview.chromium.org/2229503002
Cr-Commit-Position: refs/heads/master@{#410472}
Commit  : 798b5cdff6f50fa0d19a198f5f6e3f57425fbf05
Date    : Mon Aug 08 22:08:09 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@410470  23432.0  0.0      8  good
chromium@410471  23432.0  0.0      5  good
chromium@410472  23918.0  196.553  8  bad    <--
chromium@410473  23873.6  246.862  5  bad
chromium@410476  23873.6  246.862  5  bad
chromium@410480  23984.0  0.0      5  bad

Bisect job ran on: win_8_perf_bisect
Bug ID: 635922

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop
Test Metric: load_tools-memory:chrome:all_processes:reported_by_chrome:font_caches:effective_size_avg/load_tools_dropbox
Relative Change: 2.36%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisect/builds/2110
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004795884397003600


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

| 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, Aug 12 2016

Cc: -r...@opera.com petrcermak@chromium.org
I don't understand how my change could possibly affect font cache sizes.

petrcermak@: I see your name in the log for the relevant tests. Any ideas how to proceed?

rune: I'm going to run one more bisect to confirm that this is due to your patch.

Assuming the bisect confirms the current result, then I suggest you have a look at the traces generated by the bisect before and after your patch (I'll help you get a hold of them). If there's nothing strange, then I think we should mark this as WontFix, because it's a <1 KiB regression.

Comment 7 by r...@opera.com, Aug 12 2016

Btw, the test does not have an owner according to appspot. Perhaps you could add yourself or the appropriate owner to the test(s)?

The owner list seems to be Google internal. At least, it goes to go/ if I try to "edit owners" from appspot.

What do you mean by "according to appspot"?

Comment 9 by r...@opera.com, Aug 12 2016

The first url has an empty "owners:" while the second says "owners: rune@opera.com":

https://chromeperf.appspot.com/group_report?bug_id=635922
https://chromeperf.appspot.com/group_report?bug_id=618674

I don't know where the owners are recorded, but I gathered they could be changed through "Other Pages => Edit Test Owners".

Cc: sullivan@chromium.org
My name is in the spreadsheet.

+cc sullivan: Any idea why my name doesn't show up in https://chromeperf.appspot.com/group_report?bug_id=635922?
The dashboard 'owners' feature is pretty broken, we need to redo it. Currently the spreadsheet (go/perf-owners) is manual.

Comment 12 by r...@opera.com, Aug 12 2016

I have access to:

docs.google.com/spreadsheets/d/1R_1BAOd3xeVtR0jn6wB5HHJ2K25mIbKp3iIRQKkX38o/

Does go/perf-owners redirect to that sheet, or is it different?

Yes, it's that sheet.

Comment 14 by r...@opera.com, Aug 12 2016

Thanks! It would be good if that link on appspot was accessible to non-Googlers.

Re #14: I replaced the outdated owners on appspot with a link to the spreadsheet.
Project Member

Comment 16 by 42576172...@developer.gserviceaccount.com, Aug 13 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Style invalidation support for :first/last/only-child.
Author  : rune
Commit description:
  
Got rid of SubtreeStyleChanges for those pseudo classes. Also fixed
issue 245914 by looking at next/previous element, not node, to figure
out if we are the first. The code in checkForSiblingStyleChanges could
be restructured quite a bit and made simpler now have changedElement
(changedNode => changeElement since the input is always an element).

BUG=245914

Review-Url: https://codereview.chromium.org/2229503002
Cr-Commit-Position: refs/heads/master@{#410472}
Commit  : 798b5cdff6f50fa0d19a198f5f6e3f57425fbf05
Date    : Mon Aug 08 22:08:09 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@410470  23432.0  0.0      8  good
chromium@410471  23443.0  31.1127  8  good
chromium@410472  23873.6  246.862  5  bad    <--
chromium@410474  23873.6  246.862  5  bad
chromium@410478  23873.6  246.862  5  bad
chromium@410486  23873.6  246.862  5  bad
chromium@410509  23891.2  207.507  5  bad

Bisect job ran on: win_8_perf_bisect
Bug ID: 635922

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop
Test Metric: load_tools-memory:chrome:all_processes:reported_by_chrome:font_caches:effective_size_avg/load_tools_dropbox
Relative Change: 1.96%
Score: 95.0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisect/builds/2114
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004545383173362480


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

| 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 17 by r...@opera.com, Aug 15 2016

The second bisect still points to my change.

The only way I can think that my change could affect the font cache size would be if some page in the set re-calculate style for fewer elements in the presence of matching :first-child/:last-child/:only-child selectors, and that some fonts' data would not be accessed during the recalc that otherwise would have, and that this somehow affects the font cache.

I don't have access to any Windows machines to test with.

We tear down the browser after every single page, so the regression would have to happen within the Dropbox page.

To confirm your hypothesis, could you please prepare a patch that does some sort of instrumentation and submit it as a tryjob to the windows bots?
Project Member

Comment 20 by 42576172...@developer.gserviceaccount.com, Aug 16 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Style invalidation support for :first/last/only-child.
Author  : rune
Commit description:
  
Got rid of SubtreeStyleChanges for those pseudo classes. Also fixed
issue 245914 by looking at next/previous element, not node, to figure
out if we are the first. The code in checkForSiblingStyleChanges could
be restructured quite a bit and made simpler now have changedElement
(changedNode => changeElement since the input is always an element).

BUG=245914

Review-Url: https://codereview.chromium.org/2229503002
Cr-Commit-Position: refs/heads/master@{#410472}
Commit  : 798b5cdff6f50fa0d19a198f5f6e3f57425fbf05
Date    : Mon Aug 08 22:08:09 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N   Good?
chromium@410470  23439.3  25.4034  12  good
chromium@410471  23432.0  0.0      5   good
chromium@410472  23873.6  246.862  5   bad    <--
chromium@410474  23984.0  0.0      5   bad
chromium@410478  23800.0  271.786  12  bad
chromium@410486  23987.0  8.48528  8   bad
chromium@410509  23873.6  246.862  5   bad

Bisect job ran on: win_8_perf_bisect
Bug ID: 635922

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop
Test Metric: load_tools-memory:chrome:all_processes:reported_by_chrome:font_caches:effective_size_avg/load_tools_dropbox
Relative Change: 1.88%
Score: 98.0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisect/builds/2116
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004241097308193728


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

| 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 21 by r...@opera.com, Aug 16 2016

I tried reverting:

crrev.com/798b5cdf (410472)
crrev.com/44689f75
crrev.com/1ba55572

The two extra reverts were changes on top of 410472, so necessary for clean revert.

Then ran it the win-8 try-bot:

https://codereview.chromium.org/2254443002/

The reverts did not cause any improvements for font cache avg size for dropbox:

http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-08-16_08-10-20

rune: Thanks a lot for the investigaiton. Could you please try reverting all the way to 410471 (before your patch) and then running the tryjob (to check that the perf bot can actually reproduce the regression)?
Labels: SystemHealth-Sheriff
Labels: Hotlist-SystemHealthBankruptcy
Status: Archived (was: Assigned)
Temporarily declaring bankruptcy on the *desktop* system health benchmark.
The number of alerts became unmanageable and the overall process needs to be improved to make it sustainable.
The alerts have been turned off and I'm archiving the outstanding regressions.
Note: this is just about desktop, the mobile system health stays up. 

Sign in to add a comment