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

Issue 659647 link

Starred by 4 users
Status: Fixed
Owner:
█ bugsnash@chromium.org
OOO until 29th Jan
Closed: Nov 2016
Cc:
m...@chromium.org
█ nainar@chromium.org
esprehn@chromium.org
style-bugs@google.com
chrome-perf-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 659535



Sign in to add a comment

rebuildLayoutTree performance regression (nextTextSibling)

Reported by r...@opera.com, Oct 26 2016 
While investigating  issue 659535 , I was not able to get back to the performance for PerformanceTests/DOM/select-single-remove.html as before my offending change.

Looking at callgrind output, the recalcStyle behavior was different and a lot of time is currently spent in Node::nextTextSibling. I found that the reason was crrev.com/651c1399

Now, it wasn't very noticeable in the graphs since the issue I introduced covered it, but reverting my change only brought the perf graph half the way up again. Reverting crrev.com/651c1399 made everything fine. So without my breakage, crrev.com/651c1399 would've caused nearly a 50% decrease in performance for the test in question.
rebuildLayoutTree.png
53.1 KB View Download
rebuildLayoutTree2.png
60.9 KB View Download

Comment 1 by nainar@chromium.org, Oct 27 2016

Owner: bugsnash@chromium.org
Hi Rune, 

Thank you for doing the perf analysis here. Bugs, Elliott and I just wrapped up discussing what to do here. The solution here is:

Change the HashMap on Document to:
HeapHashMap<Member<Element>, RefPtr<RecalcData>> m_recalcData

where RecalcData contains both a pointer to the non attached ComputedStyle and  a Text* to the nextTextSibling. 

I will edit the design document to reflect this as well. 
I am marking Bugs as owner as he will be working on this. Also marking 659535 as a blocker for the meta bug.

Comment 2 by nainar@chromium.org, Oct 27 2016 

Sorry I am bad at copy pasting apparently. RecalcData doesn't need to be ref counted. 
The map will be:
HeapHashMap<Member<Element>, RecalcData> m_recalcData

Comment 3 by nainar@chromium.org, Oct 27 2016

Cc: -bugsnash@chromium.org nainar@chromium.org

Comment 4 by nainar@chromium.org, Oct 28 2016

Cc: m...@chromium.org
 Issue 660181  has been merged into this issue.

Comment 5 by r...@opera.com, Oct 31 2016 

My anticipation is now reflected in the graphs: https://chromeperf.appspot.com/group_report?bug_id=660181

Comment 6 by esprehn@chromium.org, Nov 14 2016

Labels: M-56 ReleaseBlock-Stable
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 16 2016 

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

commit d1fc66d0e6f64e38bcff450b54f908b354eff2aa
Author: bugsnash <bugsnash@chromium.org>
Date: Wed Nov 16 00:45:22 2016

Fixed perf regression by removing tree traversal for text sibling.

Changed contents of HeapHashMap on Document to store a new struct
StyleRecalcData containing both the original ComputedStyle and
the next Text sibling.

Deleted Node::nextTextSibling method that traverses the tree to find
the next text sibling, replaced usage of this method with access to
the Text node from the StyleRecalcData.

Patch yields a 79.21% performance improvement to
blink_perf.dom:select-single-remove benchmark on mac-10-10 bot:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-11-06_17-33-01

BUG= 659647 

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

[modify] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/BUILD.gn
[modify] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/Element.h
[modify] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/Node.cpp
[modify] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/Node.h
[add] https://crrev.com/d1fc66d0e6f64e38bcff450b54f908b354eff2aa/third_party/WebKit/Source/core/dom/StyleReattachData.h

Comment 8 by bugsnash@chromium.org, Nov 16 2016

Status: Fixed (was: Assigned)

Comment 9 by nainar@chromium.org, Dec 4 2016 

Issue 659574 has been merged into this issue.
►

Sign in to add a comment