Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2376453004:
Stash ComputedStyles on new HeapHashMap on Document. (Closed)
|
Created:
4 years, 2 months ago by nainar Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, blink-reviews-style_chromium.org, Bugs Nash, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStash ComputedStyles on new HeapHashMap on Document.
This patch stashes ComputedStyles on new HeapHashMap on Document and
adds relevant getters/setters.
In Document::updateStyle, we clear space assigned after we are done
with Style resolution and Layout Tree Construcion phase.
The patch then adds an entry on this map in Element::recalcOwnStyle and
reads this entry in Element::rebuildLayoutTree.
Pair programmed with bugsnash@
BUG=595137
Committed: https://crrev.com/0e000934cd50d42e30d776ec50fb976f9f68b59c
Cr-Commit-Position: refs/heads/master@{#423088}
Patch Set 1 #Patch Set 2 : Replace takeNonAttachedStyle with getNonAttachedStyle and clear the HashMap at the end of layout tr… #Patch Set 3 : Remove unused functions and reserve capacity for HeapHashMap at start of Document::updateStyle() #Patch Set 4 : Add Timeout expectation on all platforms for imported/wpt/quirks-mode/unitless-length.html as a pla… #Patch Set 5 : CL for src perf tryjob to run blink_perf.svg benchmark on all platform(s) #
Total comments: 6
Patch Set 6 : Post Elliott's review #Patch Set 7 : Post Elliott's review #
Total comments: 12
Patch Set 8 : Post LGTM with nits changes #
Total comments: 6
Patch Set 9 : Post LGTM with nits changes #
Messages
Total messages: 48 (36 generated)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. Add relevant getters/setters. Use this in Element::recalcOwnStyle and Element::buildLayoutTree Revert "Storage of ComputedStyle separate from LayoutObject." This reverts commit 7f405ec2b6914d482d081d2cb5d80bdf5226bd57. Revert "Made ElementRareData store ComputedStyle on LayoutObject if possible." This reverts commit c542d3744b905a2a00f5ac2dd2047a6a323ec181. BUG= ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. Add relevant getters/setters. Use these values in Element::recalcOwnStyle and Element::buildLayoutTree Clear the HeapHashMap in Document::updateStyle. BUG=595137 ==========
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. Add relevant getters/setters. Use these values in Element::recalcOwnStyle and Element::buildLayoutTree Clear the HeapHashMap in Document::updateStyle. BUG=595137 ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. BUG=595137 ==========
nainar@chromium.org changed reviewers: + meade@chromium.org
meade@, PTAL for a first pass through. ccing bugsnash@ since they are on leave.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nainar@chromium.org changed reviewers: + esprehn@chromium.org
Hi esprehn@, There is currently one test timing out - imported/wpt/quirks-mode/unitless-length.html. I am marking it as Timeout on all platforms for the meanwhile so that I can catch any other crashes/failures on the bots while am investigating that. Could you take a look at the patch in the meanwhile? Thank you!
CL for src perf tryjob to run blink_perf.svg benchmark on all platform(s)
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. BUG=595137 ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Written with bugsnash@ Running the latest patch on perf bots here: https://codereview.chromium.org/2376453004 BUG=595137 ==========
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Written with bugsnash@ Running the latest patch on perf bots here: https://codereview.chromium.org/2376453004 BUG=595137 ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. BUG=595137 ==========
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. BUG=595137 ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Patch 4 has the try bot results and Patch 5 has the perf bot results. BUG=595137 ==========
Your reserveInitialCapacity is causing us to make an ever bigger hash map in every call to recalcStyle, that'll probably cause crashes and OOM. :P https://codereview.chromium.org/2376453004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2376453004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1261: void Document::clear() { Remove this function, you can't add something so generic as clear() to Document. https://codereview.chromium.org/2376453004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1264: m_nonAttachedStyle.remove(iter); m_nonAttachedStyle.clear(), note that calling remove() like this on a hash map is not allowed. You can't mutate the hash map while you're iterating. This is also going to be very slow since you're causing the hashmap to repeatedly resize https://codereview.chromium.org/2376453004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1898: m_nonAttachedStyle.reserveCapacityForSize(initialElementCount); initialElementCount doesn't mean what you think it does, it's the number of times we called styleForElement in recalcStyle (for all time). That number is ever increasing, which means you're creating a bigger and bigger hash map here every time. I'd leave this out
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Patch 4 has the try bot results and Patch 5 has the perf bot results. BUG=595137 ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. BUG=595137 ==========
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. BUG=595137 ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Pair programmed with bugsnash@ BUG=595137 ==========
esprehn@ Made the changes, PTAL? Thanks! https://codereview.chromium.org/2376453004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2376453004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1261: void Document::clear() { Done. https://codereview.chromium.org/2376453004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1264: m_nonAttachedStyle.remove(iter); Done. https://codereview.chromium.org/2376453004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1898: m_nonAttachedStyle.reserveCapacityForSize(initialElementCount); Done.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm w/ nits fixed. https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1254: m_nonAttachedStyle.set(element, computedStyle); &element https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1258: return m_nonAttachedStyle.get(element); &element https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:331: void addNonAttachedStyle(Element*, RefPtr<ComputedStyle>); Element& https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:332: RefPtr<ComputedStyle> getNonAttachedStyle(Element*); Element& https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1417: using NonAttachedStyleMap = I'd remove the typedef here, you don't need it
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Made the changes. Thank you for the review Elliott! :) https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1254: m_nonAttachedStyle.set(element, computedStyle); Done. https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1258: return m_nonAttachedStyle.get(element); Done. https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:331: void addNonAttachedStyle(Element*, RefPtr<ComputedStyle>); Done. https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:332: RefPtr<ComputedStyle> getNonAttachedStyle(Element*); Done. https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1417: using NonAttachedStyleMap = Done. https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1949: document().addNonAttachedStyle(this, std::move(newStyle)); Changed to -> *this https://codereview.chromium.org/2376453004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1994: reattachContext.resolvedStyle = document().getNonAttachedStyle(this).get(); Changed to -> *this
https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1258: return m_nonAttachedStyle.get(&element); .get() https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:332: RefPtr<ComputedStyle> getNonAttachedStyle(Element&); This should return ComputedStyle* btw, you're not returning ownership, the map keeps that. = https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1994: reattachContext.resolvedStyle = document().getNonAttachedStyle(*this).get(); -get
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Made all the changes. Confirmed with meade@ that it is OK to land. https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1258: return m_nonAttachedStyle.get(&element); Done. https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:332: RefPtr<ComputedStyle> getNonAttachedStyle(Element&); Done. https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2376453004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1994: reattachContext.resolvedStyle = document().getNonAttachedStyle(*this).get(); Done.
The CQ bit was unchecked by nainar@chromium.org
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2376453004/#ps160001 (title: "Post LGTM with nits changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Pair programmed with bugsnash@ BUG=595137 ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Pair programmed with bugsnash@ BUG=595137 Committed: https://crrev.com/0e000934cd50d42e30d776ec50fb976f9f68b59c Cr-Commit-Position: refs/heads/master@{#423088} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0e000934cd50d42e30d776ec50fb976f9f68b59c Cr-Commit-Position: refs/heads/master@{#423088}
Message was sent while issue was closed.
Description was changed from ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. When we enter Document::updateStyle, we first reserve capacity in the map and then clear it after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Pair programmed with bugsnash@ BUG=595137 Committed: https://crrev.com/0e000934cd50d42e30d776ec50fb976f9f68b59c Cr-Commit-Position: refs/heads/master@{#423088} ========== to ========== Stash ComputedStyles on new HeapHashMap on Document. This patch stashes ComputedStyles on new HeapHashMap on Document and adds relevant getters/setters. In Document::updateStyle, we clear space assigned after we are done with Style resolution and Layout Tree Construcion phase. The patch then adds an entry on this map in Element::recalcOwnStyle and reads this entry in Element::rebuildLayoutTree. Pair programmed with bugsnash@ BUG=595137 Committed: https://crrev.com/0e000934cd50d42e30d776ec50fb976f9f68b59c Cr-Commit-Position: refs/heads/master@{#423088} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
