New issue
Advanced search Search tips

Issue 679511 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

10% regression in blink_perf.css at 441275:441328

Project Member Reported by benhenry@google.com, Jan 9 2017

Issue description

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

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


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

android-nexus7v2
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jan 10 2017

Cc: timloh@chromium.org
Owner: timloh@chromium.org

=== Auto-CCing suspected CL author timloh@chromium.org ===

Hi timloh@chromium.org, 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 : timloh
  Commit : ed0f6121435448aa316c90be1468398ab2ef71b4
  Date   : Wed Jan 04 03:49:42 2017
  Subject: Disallow setting invalid values for registered properties via CSSOM

Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : blink_perf.css
  Metric       : CSSPropertySetterGetterMethods/CSSPropertySetterGetterMethods
  Change       : 7.01% | 416.92268723 -> 387.709572622

Revision             Result                  N
chromium@441274      416.923 +- 65.4449      30      good
chromium@441301      421.369 +- 28.809       30      good
chromium@441308      425.976 +- 44.3575      30      good
chromium@441312      417.459 +- 30.1937      30      good
chromium@441313      378.139 +- 23.7062      30      bad       <--
chromium@441314      379.962 +- 21.6213      30      bad
chromium@441315      389.93 +- 23.3286       30      bad
chromium@441328      387.71 +- 25.611        30      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.css

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8990905873106299680

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5904120818434048


| 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 timloh@chromium.org, Jan 10 2017

Bisect looks accurate. Maybe the extra cost of bindings for the execution context plus passing it around? :/
Cc: tdres...@chromium.org
Are there measurable tradeoffs for your CL? If we could justify this with an improvement in performance in some other area, we can negotiate. Also adding Tim in case he has ideas.

Comment 6 by timloh@chromium.org, Jan 10 2017

Status: Assigned (was: Untriaged)
I might just revert this for now and then investigate :/

Comment 7 by timloh@chromium.org, Jan 11 2017

Components: Blink>CSS
Was sheriffing today/yesterday but will investigate and revert or try to fix tomorrow.

Comment 8 by timloh@chromium.org, Jan 12 2017

Patch to fix is up for review https://codereview.chromium.org/2625303002/, perf bot suggests it fixed the regression.
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 12 2017

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

commit 2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521
Author: timloh <timloh@chromium.org>
Date: Thu Jan 12 09:00:32 2017

Fix CSSPropertySetterGetterMethods regression

A recent patch crrev.com/2607403002 introduced a 10% regression in
setters and getters for CSSOM style objects. This patch fixes this
by removing the additional binding to access the registry, instead
using already existing members to access it.

BUG= 679511 

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

[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.h
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/css/CSSStyleDeclaration.h
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/css/PropertySetCSSStyleDeclaration.cpp
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/css/PropertySetCSSStyleDeclaration.h
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/editing/VisiblePositionTest.cpp
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/editing/commands/RemoveCSSPropertyCommand.cpp
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp
[modify] https://crrev.com/2dcda31b3f6d9e8cf588f0c233d9c8c7cb9b4521/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp

Thanks!
Status: Fixed (was: Assigned)

Sign in to add a comment