Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(189)

Issue 18276003: Avoid N^2 walk placing renderers when building the render tree (Closed)

Created:
7 years, 5 months ago by leviw_travelin_and_unemployed
Modified:
7 years, 5 months ago
Reviewers:
ojan, esprehn, eseidel
CC:
blink-reviews, webcomponents-bugzilla_chromium.org, eae+blinkwatch, dglazkov+blink, apavlov+blink_chromium.org, adamk+blink_chromium.org, darktears
Visibility:
Public.

Description

Avoid N^2 walk placing renderers when building the render tree Blink resolves style by walking each element from first children to last, but when we go to insert their renderer, we look for the next renderer to insert before. On the initial style resolve, this means we'll walk the entire DOM tree trying to find the right renderer to insert before leading to an N^2 loop over the DOM on load. This could be fixed by changing the semantics by which we insert renderers (insert after instead of insert before). Instead, here I reverse the order we resolve style. This should ensure that in the common case, we'll find the renderer to insert before immediately. While looking at this, I also found we have an N^2 loop for resolving Nth last child selectors, and reversing the style resolve loop would have caused the same issue for Nth child selectors. To fix prevent this regression, I'm piping the child index down to the style resolver in the common case. This ensures both Nth and Nth last should usually be O(1). The previous version of this patch resulted in a perf regression due to the extra in-order loop in Element::recalcStyle. This loop is now gated on the presence of child or sibling selectors. The common case should be faster. Previous code review: https://chromiumcodereview.appspot.com/15871005 BUG=245478 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154053

Patch Set 1 #

Patch Set 2 : Merged to ToT #

Patch Set 3 : Trying again since the trybots seem unhappy for reasons unknown... #

Patch Set 4 : Adding test expectations #

Total comments: 9

Patch Set 5 : Remove unneeded changes and rebaseline following Elliott's whitespace changes. #

Patch Set 6 : Removing extra parens (whoops) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+114 lines, -76 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
M LayoutTests/platform/linux/fast/html/details-replace-text-expected.txt View 1 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/platform/win/fast/html/details-add-child-2-expected.txt View 1 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/platform/win/fast/html/details-open2-expected.txt View 1 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/css/SelectorChecker.h View 3 chunks +3 lines, -1 line 0 comments Download
M Source/core/css/SelectorChecker.cpp View 1 2 3 4 2 chunks +4 lines, -2 lines 0 comments Download
M Source/core/css/SiblingTraversalStrategies.h View 1 2 3 4 2 chunks +14 lines, -9 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.h View 1 2 3 4 1 chunk +7 lines, -1 line 0 comments Download
M Source/core/css/resolver/StyleResolver.cpp View 1 2 3 4 9 chunks +11 lines, -11 lines 0 comments Download
M Source/core/css/resolver/StyleResolverState.h View 1 2 3 4 3 chunks +5 lines, -3 lines 0 comments Download
M Source/core/css/resolver/StyleResolverState.cpp View 1 2 3 4 3 chunks +4 lines, -3 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/dom/Element.h View 1 2 3 4 2 chunks +3 lines, -3 lines 0 comments Download
M Source/core/dom/Element.cpp View 1 2 3 4 5 3 chunks +28 lines, -24 lines 0 comments Download
M Source/core/dom/Node.h View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/dom/Node.cpp View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/dom/Text.cpp View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/dom/shadow/ShadowRoot.cpp View 1 2 3 4 1 chunk +6 lines, -13 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
leviw_travelin_and_unemployed
7 years, 5 months ago (2013-07-10 22:46:52 UTC) #1
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
7 years, 5 months ago (2013-07-11 01:04:28 UTC) #2
esprehn
Reversing the notifier loop and the RAII thing doesn't seem right, lets understand what's going ...
7 years, 5 months ago (2013-07-11 01:24:32 UTC) #3
leviw_travelin_and_unemployed
https://chromiumcodereview.appspot.com/18276003/diff/10001/Source/core/dom/ContainerNodeAlgorithms.h File Source/core/dom/ContainerNodeAlgorithms.h (right): https://chromiumcodereview.appspot.com/18276003/diff/10001/Source/core/dom/ContainerNodeAlgorithms.h#newcode229 Source/core/dom/ContainerNodeAlgorithms.h:229: m_postInsertionNotificationTargets[i - 1]->didNotifySubtreeInsertions(m_insertionPoint); On 2013/07/11 01:24:32, esprehn wrote: > ...
7 years, 5 months ago (2013-07-11 01:35:11 UTC) #4
leviw_travelin_and_unemployed
ptal
7 years, 5 months ago (2013-07-11 19:14:57 UTC) #5
esprehn
On 2013/07/11 19:14:57, Levi wrote: > ptal LGTM This looks great, I wonder what reversing ...
7 years, 5 months ago (2013-07-11 19:36:55 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leviw@chromium.org/18276003/24001
7 years, 5 months ago (2013-07-11 20:28:13 UTC) #7
commit-bot: I haz the power
7 years, 5 months ago (2013-07-11 22:56:51 UTC) #8
Message was sent while issue was closed.
Change committed as 154053

Powered by Google App Engine
This is Rietveld 408576698