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

Issue 693458 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

13.1% regression in blink_perf.dom at 450321:450381

Project Member Reported by alexclarke@chromium.org, Feb 17 2017

Issue description

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

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


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

chromium-rel-win7-gpu-intel
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Feb 17 2017

Cc: yosin@chromium.org
Owner: yosin@chromium.org

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

Hi yosin@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 : yosin
  Commit : d892f9592860691ae9a782c12260c94ed6bd1a63
  Date   : Tue Feb 14 15:56:00 2017
  Subject: Make FrameSelection to hold non-canonicalized DOM positions

Bisect Details
  Configuration: winx64intel_perf_bisect
  Benchmark    : blink_perf.dom
  Metric       : inner_html_with_selection/inner_html_with_selection
  Change       : 11.61% | 0.538333333333 -> 0.600833333333

Revision             Result                     N
chromium@450320      0.538333 +- 0.0154704      6      good
chromium@450351      0.5475 +- 0.0184255        6      good
chromium@450366      0.543167 +- 0.0182985      6      good
chromium@450368      0.549333 +- 0.0238188      6      good
chromium@450369      0.546333 +- 0.0230507      6      good
chromium@450370      0.604833 +- 0.0118673      6      bad       <--
chromium@450374      0.599167 +- 0.0185158      6      bad
chromium@450381      0.600833 +- 0.0124432      6      bad

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

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

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


| 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 Speed>Bisection.  Thank you!

Comment 4 by yosin@chromium.org, Feb 17 2017

Components: Blink>Editing>Selection
Labels: -Performance-Sheriff Performance
Status: Started (was: Untriaged)

Comment 5 by yosin@chromium.org, Feb 18 2017

SeletionEditor::markCacheDirty occupies 8.56% of ContainerNode::appendChild.
(VS default constructor takes 6.48%).

Give this, markCacheDirty() should call VS default ctor only if |m_cacheIsDirty| is false.


Comment 6 by yosin@chromium.org, Feb 20 2017

In review: http://crrev.com/2708533002
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 20 2017

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

commit 54210329887cfd683e71722b93a5e05b95f5e12d
Author: yosin <yosin@chromium.org>
Date: Mon Feb 20 05:53:45 2017

Make SelectionEditor::markCacheDirty() not to reset cached value if it has been cleared

This patch makes |SelectionEditor::markCacheDirty()| not to reset cached
|VisibleSeleciton| and |VisibleSelectionInFlatTree| when we've already cleard,
calling default constructor of |VisibleSelection| and
|VisibleSelectionInFlatTree| and copying them, to make |markCacheDirty()|
faster.

In "PerformanceTests/Bindings/append-child.html", |markCacheDirty()| takes
5.26% of |ContainerNode::appendChild()|.

In "PerformanceTests/DOM/inner_html_with_selection.html", |markCacheDirty()|
takes 8.56% of |ContainerNode::appendChild()|.

BUG= 692939 , 693458 
TEST=n/a; no behavior changes

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

[modify] https://crrev.com/54210329887cfd683e71722b93a5e05b95f5e12d/third_party/WebKit/Source/core/editing/SelectionEditor.cpp

Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Feb 21 2017


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

Hi yosin@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 : yosin
  Commit : 54210329887cfd683e71722b93a5e05b95f5e12d
  Date   : Mon Feb 20 05:53:45 2017
  Subject: Make SelectionEditor::markCacheDirty() not to reset cached value if it has been cleared

Bisect Details
  Configuration: winx64intel_perf_bisect
  Benchmark    : blink_perf.dom
  Metric       : inner_html_with_selection/inner_html_with_selection
  Change       : 8.40% | 0.619333333333 -> 0.567333333333

Revision             Result                     N
chromium@451555      0.619333 +- 0.0140475      6      good
chromium@451564      0.637 +- 0.0601997         6      good
chromium@451566      0.6225 +- 0.0164165        6      good
chromium@451567      0.567833 +- 0.0128387      6      bad       <--
chromium@451568      0.5705 +- 0.0139104        6      bad
chromium@451572      0.567667 +- 0.0199833      6      bad
chromium@451588      0.567333 +- 0.0222111      6      bad

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

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

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


| 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 Speed>Bisection.  Thank you!

Comment 10 by yosin@chromium.org, Feb 23 2017

Status: Fixed (was: Started)
Recover some: 0.62 to 0.56, base is 0.54.

Sign in to add a comment