Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2317573003:
Introduce ContainerNode::insertNodeVector() to share code in insertBefore(), appendChild(), and rep… (Closed)
DescriptionIntroduce ContainerNode::insertNodeVector() to share code in insertBefore(), appendChild(), and replaceChild().
This CL has no behavior changes.
This CL is a preparation to fix crbug.com/523157.
BUG=523157
Committed: https://crrev.com/a81fa6e7649b2534d7797cc6232eecbfdfe76e85
Cr-Commit-Position: refs/heads/master@{#416891}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Functor #Patch Set 3 : Update a copied comment; "this" -> "container" #
Messages
Total messages: 24 (16 generated)
The CQ bit was checked by tkent@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 ========== Introduce ContainerNode::insertNodeVector() to share code in insertBeofre(), appendChild(), and replaceChild(). This CL has no behavior changes. This CL is a preparation to fix crbug.com/523157. BUG=523157 ========== to ========== Introduce ContainerNode::insertNodeVector() to share code in insertBeofre(), appendChild(), and replaceChild(). This CL has no behavior changes, but might have performance impact in DOM micro benchmarks. This CL is a preparation to fix crbug.com/523157. BUG=523157 ==========
tkent@chromium.org changed reviewers: + hayato@chromium.org
hayato@, would you review this please? https://codereview.chromium.org/2317573003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2317573003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ContainerNode.cpp:153: void ContainerNode::insertNodeVector(const NodeVector& targets, Node* next, bool (ContainerNode::*mutator)(Node& child, Node* next)) Functor is a smarter solution. But I feel functor is overkill in this case.
https://codereview.chromium.org/2317573003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2317573003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ContainerNode.cpp:153: void ContainerNode::insertNodeVector(const NodeVector& targets, Node* next, bool (ContainerNode::*mutator)(Node& child, Node* next)) I see. However, I'm afraid that `mutator` can not be inlined, though Functor with template can be inlined, I think. I am not sure which is better in this case. It looks a cost/benefits issue. Could we measure the performance difference between them before landing the CL?
The CQ bit was unchecked by tkent@chromium.org
CL for src perf tryjob to run blink_perf.dom benchmark on all platform(s)
The CQ bit was checked by tkent@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...
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2317573003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2317573003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ContainerNode.cpp:153: void ContainerNode::insertNodeVector(const NodeVector& targets, Node* next, bool (ContainerNode::*mutator)(Node& child, Node* next)) On 2016/09/07 at 04:56:50, hayato wrote: > I see. However, I'm afraid that `mutator` can not be inlined, though Functor with template can be inlined, I think. > > I am not sure which is better in this case. It looks a cost/benefits issue. > Could we measure the performance difference between them before landing the CL? I observed no performance differences with dromaeo.domcoremodify and macOS official build. However, we might have regressions on other configurations. So, I have changed my mind. Patch Set 2 applies functors.
Description was changed from ========== Introduce ContainerNode::insertNodeVector() to share code in insertBeofre(), appendChild(), and replaceChild(). This CL has no behavior changes, but might have performance impact in DOM micro benchmarks. This CL is a preparation to fix crbug.com/523157. BUG=523157 ========== to ========== Introduce ContainerNode::insertNodeVector() to share code in insertBeofre(), appendChild(), and replaceChild(). This CL has no behavior changes. This CL is a preparation to fix crbug.com/523157. BUG=523157 ==========
LGTM. Thank you updating the CL.
The CQ bit was checked by tkent@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 ========== Introduce ContainerNode::insertNodeVector() to share code in insertBeofre(), appendChild(), and replaceChild(). This CL has no behavior changes. This CL is a preparation to fix crbug.com/523157. BUG=523157 ========== to ========== Introduce ContainerNode::insertNodeVector() to share code in insertBefore(), appendChild(), and replaceChild(). This CL has no behavior changes. This CL is a preparation to fix crbug.com/523157. BUG=523157 ==========
The CQ bit was unchecked by tkent@chromium.org
The CQ bit was checked by tkent@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org Link to the patchset: https://codereview.chromium.org/2317573003/#ps60001 (title: "Update a copied comment; "this" -> "container"")
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 #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Introduce ContainerNode::insertNodeVector() to share code in insertBefore(), appendChild(), and replaceChild(). This CL has no behavior changes. This CL is a preparation to fix crbug.com/523157. BUG=523157 ========== to ========== Introduce ContainerNode::insertNodeVector() to share code in insertBefore(), appendChild(), and replaceChild(). This CL has no behavior changes. This CL is a preparation to fix crbug.com/523157. BUG=523157 Committed: https://crrev.com/a81fa6e7649b2534d7797cc6232eecbfdfe76e85 Cr-Commit-Position: refs/heads/master@{#416891} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a81fa6e7649b2534d7797cc6232eecbfdfe76e85 Cr-Commit-Position: refs/heads/master@{#416891} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
