Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1590143002:
removeBetween() -> detach() performance fix. (Closed)
|
Created:
4 years, 11 months ago by rune Modified:
4 years, 11 months ago Reviewers:
esprehn CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionremoveBetween() -> detach() performance fix.
In [1] we started to persist invalidation sets on elements getting
reattach style change because sibling invalidation sets scheduled on
detached elements still need to be processed for attached siblings.
However, the invalidation sets need to be cleared when such elements
are removed from the document tree. Clearing that invalidation set were
done with a detach() which also would go through detach() on an
already detached subtree. That caused a performance regression in the
the blink_perf.dom:select-single-add micro benchmark.
Instead of brute forcing with detach(), we clear the invalidation sets
for the elements of the disconnected subtree in Element::removedFrom().
[1] https://codereview.chromium.org/1533683002
R=esprehn@chromium.org
BUG=577439
TEST=PerformanceTests/DOM/select-single-add.html
Committed: https://crrev.com/658484182e5782d3354edb4ca40be18e93f98e30
Cr-Commit-Position: refs/heads/master@{#369798}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebased #Patch Set 3 : Clear invalidation in Element::removedFrom. #
Total comments: 2
Patch Set 4 : Check needsStyleInvalidation() before clearing. #
Messages
Total messages: 23 (10 generated)
The CQ bit was checked by rune@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1590143002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1590143002/1
Description was changed from ========== removeBetween() -> detach() performance fix. In [1] we started to persist invalidation sets on elements getting reattach style change because sibling invalidation sets scheduled on detached elements still need to be processed for attached siblings. However, the invalidation sets need to be cleared when such elements are removed from the document tree. Clearing that invalidation set were done with an detach() which also would go through detach() on an already detached subtree. That caused a performance regression in the the blink_perf.dom:select-single-add micro benchmark. Instead of brute forcing with detach(), we clear the invalidation set for the root element of the disconnected subtree. [1] https://codereview.chromium.org/1533683002 R=esprehn@chromium.org BUG=577439 ========== to ========== removeBetween() -> detach() performance fix. In [1] we started to persist invalidation sets on elements getting reattach style change because sibling invalidation sets scheduled on detached elements still need to be processed for attached siblings. However, the invalidation sets need to be cleared when such elements are removed from the document tree. Clearing that invalidation set were done with an detach() which also would go through detach() on an already detached subtree. That caused a performance regression in the the blink_perf.dom:select-single-add micro benchmark. Instead of brute forcing with detach(), we clear the invalidation set for the root element of the disconnected subtree. [1] https://codereview.chromium.org/1533683002 R=esprehn@chromium.org BUG=577439 TEST=PerformanceTests/DOM/select-single-add.html ==========
https://codereview.chromium.org/1590143002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/1590143002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ContainerNode.cpp:612: document().styleEngine().styleInvalidator().clearInvalidation(toElement(oldChild)); can Element::removedFrom do this? It seems like it belongs there with the other Element clean up code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1590143002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/1590143002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ContainerNode.cpp:612: document().styleEngine().styleInvalidator().clearInvalidation(toElement(oldChild)); On 2016/01/15 00:31:45, esprehn wrote: > can Element::removedFrom do this? It seems like it belongs there with the other > Element clean up code. Yeah, I think so. It means it'll check needsStyleInvalidation() both in detach() and removedFrom(), for the non-root elements, in the case where we're removing the element from the document, but that's probably ok (clearInvalidation() only called once, of course).
The CQ bit was checked by rune@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1590143002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1590143002/40001
Description was changed from ========== removeBetween() -> detach() performance fix. In [1] we started to persist invalidation sets on elements getting reattach style change because sibling invalidation sets scheduled on detached elements still need to be processed for attached siblings. However, the invalidation sets need to be cleared when such elements are removed from the document tree. Clearing that invalidation set were done with an detach() which also would go through detach() on an already detached subtree. That caused a performance regression in the the blink_perf.dom:select-single-add micro benchmark. Instead of brute forcing with detach(), we clear the invalidation set for the root element of the disconnected subtree. [1] https://codereview.chromium.org/1533683002 R=esprehn@chromium.org BUG=577439 TEST=PerformanceTests/DOM/select-single-add.html ========== to ========== removeBetween() -> detach() performance fix. In [1] we started to persist invalidation sets on elements getting reattach style change because sibling invalidation sets scheduled on detached elements still need to be processed for attached siblings. However, the invalidation sets need to be cleared when such elements are removed from the document tree. Clearing that invalidation set were done with a detach() which also would go through detach() on an already detached subtree. That caused a performance regression in the the blink_perf.dom:select-single-add micro benchmark. Instead of brute forcing with detach(), we clear the invalidation sets for the elements of the disconnected subtree in Element::removedFrom(). [1] https://codereview.chromium.org/1533683002 R=esprehn@chromium.org BUG=577439 TEST=PerformanceTests/DOM/select-single-add.html ==========
https://codereview.chromium.org/1590143002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/1590143002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ContainerNode.cpp:612: document().styleEngine().styleInvalidator().clearInvalidation(toElement(oldChild)); On 2016/01/15 09:42:41, rune wrote: > On 2016/01/15 00:31:45, esprehn wrote: > > can Element::removedFrom do this? It seems like it belongs there with the > other > > Element clean up code. > > Yeah, I think so. It means it'll check needsStyleInvalidation() both in detach() > and removedFrom(), for the non-root elements, in the case where we're removing > the element from the document, but that's probably ok (clearInvalidation() only > called once, of course). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Lgtm, I think you want a bit check though. https://codereview.chromium.org/1590143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1590143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1534: document().styleEngine().styleInvalidator().clearInvalidation(*this); Can we do a bit check here to avoid this long chain of function calls?
https://codereview.chromium.org/1590143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1590143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1534: document().styleEngine().styleInvalidator().clearInvalidation(*this); On 2016/01/15 17:07:24, esprehn wrote: > Can we do a bit check here to avoid this long chain of function calls? Done.
The CQ bit was checked by rune@opera.com
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/1590143002/#ps60001 (title: "Check needsStyleInvalidation() before clearing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1590143002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1590143002/60001
Message was sent while issue was closed.
Description was changed from ========== removeBetween() -> detach() performance fix. In [1] we started to persist invalidation sets on elements getting reattach style change because sibling invalidation sets scheduled on detached elements still need to be processed for attached siblings. However, the invalidation sets need to be cleared when such elements are removed from the document tree. Clearing that invalidation set were done with a detach() which also would go through detach() on an already detached subtree. That caused a performance regression in the the blink_perf.dom:select-single-add micro benchmark. Instead of brute forcing with detach(), we clear the invalidation sets for the elements of the disconnected subtree in Element::removedFrom(). [1] https://codereview.chromium.org/1533683002 R=esprehn@chromium.org BUG=577439 TEST=PerformanceTests/DOM/select-single-add.html ========== to ========== removeBetween() -> detach() performance fix. In [1] we started to persist invalidation sets on elements getting reattach style change because sibling invalidation sets scheduled on detached elements still need to be processed for attached siblings. However, the invalidation sets need to be cleared when such elements are removed from the document tree. Clearing that invalidation set were done with a detach() which also would go through detach() on an already detached subtree. That caused a performance regression in the the blink_perf.dom:select-single-add micro benchmark. Instead of brute forcing with detach(), we clear the invalidation sets for the elements of the disconnected subtree in Element::removedFrom(). [1] https://codereview.chromium.org/1533683002 R=esprehn@chromium.org BUG=577439 TEST=PerformanceTests/DOM/select-single-add.html ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== removeBetween() -> detach() performance fix. In [1] we started to persist invalidation sets on elements getting reattach style change because sibling invalidation sets scheduled on detached elements still need to be processed for attached siblings. However, the invalidation sets need to be cleared when such elements are removed from the document tree. Clearing that invalidation set were done with a detach() which also would go through detach() on an already detached subtree. That caused a performance regression in the the blink_perf.dom:select-single-add micro benchmark. Instead of brute forcing with detach(), we clear the invalidation sets for the elements of the disconnected subtree in Element::removedFrom(). [1] https://codereview.chromium.org/1533683002 R=esprehn@chromium.org BUG=577439 TEST=PerformanceTests/DOM/select-single-add.html ========== to ========== removeBetween() -> detach() performance fix. In [1] we started to persist invalidation sets on elements getting reattach style change because sibling invalidation sets scheduled on detached elements still need to be processed for attached siblings. However, the invalidation sets need to be cleared when such elements are removed from the document tree. Clearing that invalidation set were done with a detach() which also would go through detach() on an already detached subtree. That caused a performance regression in the the blink_perf.dom:select-single-add micro benchmark. Instead of brute forcing with detach(), we clear the invalidation sets for the elements of the disconnected subtree in Element::removedFrom(). [1] https://codereview.chromium.org/1533683002 R=esprehn@chromium.org BUG=577439 TEST=PerformanceTests/DOM/select-single-add.html Committed: https://crrev.com/658484182e5782d3354edb4ca40be18e93f98e30 Cr-Commit-Position: refs/heads/master@{#369798} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/658484182e5782d3354edb4ca40be18e93f98e30 Cr-Commit-Position: refs/heads/master@{#369798} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
