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

Issue 618674 link

Starred by 2 users
Status: Fixed
Owner:
█ sashab@chromium.org
Last visit > 30 days ago
Closed: Oct 2016
Cc:
█ pmeenan@chromium.org
█ r...@opera.com
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

11.8%-13.7% regression in blink_perf.css at 397457:398038

Project Member Reported by pmeenan@chromium.org, Jun 9 2016 
See the link to graphs below.

Comment 1 by pmeenan@chromium.org, Jun 9 2016 

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg3OO8qQkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgvOybtQoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgvNjMtQkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg3P-UswoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgvKrlrAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgvOybtQkM


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

chromium-rel-win10
chromium-rel-win7-gpu-ati
chromium-rel-win7-gpu-intel
chromium-rel-win7-gpu-nvidia
chromium-rel-win7-x64-dual
chromium-rel-win8-dual

Comment 2 by pmeenan@chromium.org, Jun 13 2016 

re-kicked failed bisect on different platform

Comment 3 by sullivan@chromium.org, Jun 24 2016

Cc: r...@opera.com
Trying another bisect. Also cc-ing test owner rune to get feedback on how important it is to chase this down.

Comment 4 by r...@opera.com, Jun 27 2016 

It's more than 10% regression, but a microbenchmark. Would be interesting to which commit exactly caused it.

Comment 5 by pmeenan@chromium.org, Jun 27 2016

Cc: pmeenan@chromium.org
Owner: sashab@chromium.org
sashab@ - I think this may be related to your work on const css values.  The benchmark dips when it first lands, goes back up when it is reverted and back down again when it re-lands.

Would you mind taking a look?

Comment 6 by sashab@chromium.org, Jun 28 2016 

How would adding const to a variable make things *slower*? If anything they should now be faster and use less memory/cycles. I'll investigate...

Comment 7 by sashab@chromium.org, Jun 28 2016 

Also which patch specifically are you referring to? :) Please revert it again.

Comment 8 by pmeenan@chromium.org, Jun 28 2016 

This revert: https://chromium.googlesource.com/chromium/src/+/41b2e7dad0fb637ebcf9dbd6c0e60d4bfbde48ee
and reland: https://chromium.googlesource.com/chromium/src/+/897d431a873f7369215769d55b83257b78b28671

It's possible that the improvement and then re-regression was completely independent but looking at the change log from the recovered point the other reverts didn't have matching lands/relands on either side: https://chromium.googlesource.com/chromium/src/+log/d9fcb787ca9f3cd6fba638eae31d008c62beb1e5..41b2e7dad0fb637ebcf9dbd6c0e60d4bfbde48ee

I'll try bisecting again but they haven't been completing very well and it's also possible that it's an infrastructure change which is why I'd hate to just revert to see if it fixes things.
Project Member

Comment 9 by sheriffbot@chromium.org, Jul 6 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 13 by 42576172...@developer.gserviceaccount.com, Jul 29 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@397456  3743.34  23.0322  5  good
chromium@398038  3274.22  12.9329  5  bad

Bisect job ran on: win_x64_perf_bisect
Bug ID: 618674

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.css
Test Metric: ClassInvalidation/ClassInvalidation
Relative Change: 12.53%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_x64_perf_bisect/builds/1340
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006407129132075536


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

| 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!
Project Member

Comment 14 by 42576172...@developer.gserviceaccount.com, Jul 29 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@397456  4299.92  3.52044  5  good
chromium@398038  3597.09  26.5697  5  bad

Bisect job ran on: winx64intel_perf_bisect
Bug ID: 618674

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.css
Test Metric: ClassInvalidation/ClassInvalidation
Relative Change: 16.35%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64intel_perf_bisect/builds/1037
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006407123560959664


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

| 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!
Project Member

Comment 17 by 42576172...@developer.gserviceaccount.com, Jul 29 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@397456  3743.34  23.0322  5  good
chromium@398038  3274.22  12.9329  5  bad

Bisect job ran on: win_x64_perf_bisect
Bug ID: 618674

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.css
Test Metric: ClassInvalidation/ClassInvalidation
Relative Change: 12.53%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_x64_perf_bisect/builds/1340
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006407129132075536


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

| 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!
Project Member

Comment 18 by 42576172...@developer.gserviceaccount.com, Jul 29 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@397456  4299.92  3.52044  5  good
chromium@398038  3597.09  26.5697  5  bad

Bisect job ran on: winx64intel_perf_bisect
Bug ID: 618674

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.css
Test Metric: ClassInvalidation/ClassInvalidation
Relative Change: 16.35%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64intel_perf_bisect/builds/1037
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006407123560959664


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

| 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 sullivan@chromium.org, Aug 5 2016 

Rune, several bisects are unable to reproduce this regression, even though it shows up pretty clearly on the graphs across several bots. Any ideas wht next steps we should take?
Project Member

Comment 24 by 42576172...@developer.gserviceaccount.com, Aug 7 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@397456  4369.94  25.4004  5  good
chromium@398038  3655.28  8.88325  5  bad

Bisect job ran on: winx64ati_perf_bisect
Bug ID: 618674

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.css
Test Metric: ClassInvalidation/ClassInvalidation
Relative Change: 16.35%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64ati_perf_bisect/builds/1469
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006407129132075536


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

| 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!
Project Member

Comment 25 by 42576172...@developer.gserviceaccount.com, Aug 7 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@397456  4356.73  7.03425  5  good
chromium@398038  3662.35  8.97128  5  bad

Bisect job ran on: winx64ati_perf_bisect
Bug ID: 618674

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.css
Test Metric: ClassInvalidation/ClassInvalidation
Relative Change: 15.94%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64ati_perf_bisect/builds/1470
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006407123560959664


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

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

Looking at the temporary fix in the graph, it does seem like https://bugs.chromium.org/p/chromium/issues/detail?id=618674#c5 is correct about what caused it. Did anyone look into the questioned patch to see if anything could've caused the code to become slower?

Comment 27 by sashab@chromium.org, Aug 11 2016 

It literally just adds a bunch of consts:
https://chromium.googlesource.com/chromium/src/+/897d431a873f7369215769d55b83257b78b28671%5E%21/

Do you think its the const ptr thats the problem? I can try make them const references instead, I wonder if that would dip the chart back down again.

Comment 28 by sashab@chromium.org, Aug 11 2016 

Oh, and just reading the CL description:

This patch is mostly mechanical changes, but does contain one logic
change: the static method mergeTextDecorationValues in EditingStyle
needs to be changed since it modifies a CSSValueList that points inside
the mutable style. Instead, the function is changed to take two
CSSValueLists and return a new one, and the callsite is changed to
replace the old CSSValueList with the new one in the style rather than
modifying the old CSSValueList in-place.

That would definitely have an effect on performance, but is more correct than modifying the style in-place.

Comment 29 by r...@opera.com, Aug 11 2016 

I didn't have any other indication of that being the culprit than the fact that the graph went down when it was introduced, up again when it got reverted, and down again on re-landing.

One wouldn't think EditingStyle changes should have any effects on the test in question.

Comment 30 by r...@opera.com, Aug 11 2016 

Interestingly, these are all Win regressions. Looking at the graphs for Linux, these changes are just in the noise.
Project Member

Comment 31 by bugdroid1@chromium.org, Aug 12 2016 

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

commit e6a32634cfd6854cd84dc18b0572e922e4ae6cbc
Author: sashab <sashab@chromium.org>
Date: Fri Aug 12 01:17:12 2016

Made StylePropertySet::setProperty take a const CSSValue& instead of ptr

Made StylePropertySet::setProperty take a const CSSValue& instead of a
const CSSValue*. This is more semantically correct and safe since
StylePropertySets cannot store nullptrs, and is also an attempt to fix
a strange performance issue caused from introducing const CSSValue*s.

BUG=526586, 618674 

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

[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/animation/StringKeyframe.cpp
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/css/StylePropertySet.cpp
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/css/StylePropertySet.h
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/dom/Element.h
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/editing/EditingStyle.cpp
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/editing/commands/ApplyStyleCommand.cpp
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/html/HTMLElement.cpp
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/html/HTMLHRElement.cpp
[modify] https://crrev.com/e6a32634cfd6854cd84dc18b0572e922e4ae6cbc/third_party/WebKit/Source/core/html/HTMLTableElement.cpp

Comment 32 by qyears...@chromium.org, Aug 18 2016 

Perf sheriff ping: reminder to follow up on possible performance issues
Project Member

Comment 34 by 42576172...@developer.gserviceaccount.com, Aug 22 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@397456  4065.76  3.18483  5  good
chromium@398038  3384.13  4.45249  5  bad

Bisect job ran on: winx64_10_perf_bisect
Bug ID: 618674

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.css
Test Metric: ClassInvalidation/ClassInvalidation
Relative Change: 16.77%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64_10_perf_bisect/builds/671
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006407129132075536


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

| 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 35 by jasontiller@chromium.org, Oct 7 2016

Status: Fixed (was: Assigned)
Looks like the metric recovered. Marking as fixed.
►

Sign in to add a comment