Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2822113002:
Do not set a distribution recalc flag when v1 shadow tree is added (Closed)
|
Created:
3 years, 8 months ago by hayato Modified:
3 years, 8 months ago Reviewers:
kochi CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not set a distribution recalc flag when v1 shadow tree is added
After rewriting Shadow DOM distribution engine for v1 [1], we no longer
need to set a distribution recalc flag when a v1 shadow tree is added.
A distribution recalc flag will be set correctly later, if necessary, when a
slot is inserted, removed, or other relevant DOM mutation happens.
Instead of setting recalc flag, host's children need to be lazy reattached in
adding a shadow root, which would be done in resolve assignments if we set a
distribution recalc flag.
This optimization would make loading slightly faster when there are many shadow
trees which will not have a slot because we do not need to traverse down the
trees. In the case of bug 699838, this can make loading 5ms (at most) faster.
[1] https://codereview.chromium.org/1995203002
BUG=712559
Review-Url: https://codereview.chromium.org/2822113002
Cr-Commit-Position: refs/heads/master@{#465547}
Committed: https://chromium.googlesource.com/chromium/src/+/a9c70fe4e42df88f817cb00cbc13a98b7727d99f
Patch Set 1 #Patch Set 2 : retry #Patch Set 3 : try #Patch Set 4 : try #
Messages
Total messages: 35 (25 generated)
The CQ bit was checked by hayato@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 ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. This optimization would make loading time slightly faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ========== to ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. This optimization should make loading slightly faster, especially in the case of bug 699838. [1] https://codereview.chromium.org/1995203002 BUG=712559 ==========
Description was changed from ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. This optimization should make loading slightly faster, especially in the case of bug 699838. [1] https://codereview.chromium.org/1995203002 BUG=712559 ========== to ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. This optimization should make loading slightly faster. In the case of bug 699838, this can make loading 5ms faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ==========
Description was changed from ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. This optimization should make loading slightly faster. In the case of bug 699838, this can make loading 5ms faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ========== to ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. This optimization should make loading slightly faster. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ==========
hayato@chromium.org changed reviewers: + kochi@chromium.org
PTAL
LGTM Below is more of a question than a comment: Even without this change, when no slot is in the V1 shadow root, can SlotAssignment::ResolveDistribution() be almost no-op? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/shado... ResolveDistribution() calls ResolveAssignment(), which calls DetachNotAssignedNode() for all children of shadow host. Is the time saved with this change mostly spent in DetachNotAssignedNode() then??
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hayato@chromium.org
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 hayato@chromium.org
retry
The CQ bit was checked by hayato@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...
try
The CQ bit was checked by hayato@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 ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. This optimization should make loading slightly faster. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ========== to ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. Instead of setting recalc flag, host's children need to be lazy reattached in adding a shadow root, which would be done in resolve assignments if we set a distribution recalc flag. This optimization should make loading slightly faster when there are many shadow trees which will not have a slot because we do not need to traverse down the trees. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ==========
Description was changed from ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. Instead of setting recalc flag, host's children need to be lazy reattached in adding a shadow root, which would be done in resolve assignments if we set a distribution recalc flag. This optimization should make loading slightly faster when there are many shadow trees which will not have a slot because we do not need to traverse down the trees. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ========== to ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. Instead of setting recalc flag, host's children need to be lazy reattached in adding a shadow root, which would be done in resolve assignments if we set a distribution recalc flag. This optimization should make loading slightly faster when there are many shadow trees which will not have a slot because we do not need to traverse down the trees. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ==========
Description was changed from ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. Instead of setting recalc flag, host's children need to be lazy reattached in adding a shadow root, which would be done in resolve assignments if we set a distribution recalc flag. This optimization should make loading slightly faster when there are many shadow trees which will not have a slot because we do not need to traverse down the trees. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ========== to ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. Instead of setting recalc flag, host's children need to be lazy reattached in adding a shadow root, which would be done in resolve assignments if we set a distribution recalc flag. This optimization would make loading slightly faster when there are many shadow trees which will not have a slot because we do not need to traverse down the trees. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ==========
try
The CQ bit was checked by hayato@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.
On 2017/04/18 at 08:24:33, kochi wrote: > LGTM > > Below is more of a question than a comment: > > Even without this change, when no slot is in the V1 shadow > root, can SlotAssignment::ResolveDistribution() be almost > no-op? > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/shado... > > ResolveDistribution() calls ResolveAssignment(), > which calls DetachNotAssignedNode() for all children > of shadow host. Is the time saved with this change > mostly spent in DetachNotAssignedNode() then?? No. What can be saved are the *number* of calling ResolveAssignment and tree traversing cost. RecalcDistribution is a recursive operation. We have to traverse every shadow trees, calling RessolveDistribution N times (N == the number of custom elements).
The CQ bit was checked by hayato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org Link to the patchset: https://codereview.chromium.org/2822113002/#ps60001 (title: "try")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492598156247070,
"parent_rev": "c00bae0bd50204fc323a37c28f9e77b39c8dc376", "commit_rev":
"a9c70fe4e42df88f817cb00cbc13a98b7727d99f"}
Message was sent while issue was closed.
Description was changed from ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. Instead of setting recalc flag, host's children need to be lazy reattached in adding a shadow root, which would be done in resolve assignments if we set a distribution recalc flag. This optimization would make loading slightly faster when there are many shadow trees which will not have a slot because we do not need to traverse down the trees. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 ========== to ========== Do not set a distribution recalc flag when v1 shadow tree is added After rewriting Shadow DOM distribution engine for v1 [1], we no longer need to set a distribution recalc flag when a v1 shadow tree is added. A distribution recalc flag will be set correctly later, if necessary, when a slot is inserted, removed, or other relevant DOM mutation happens. Instead of setting recalc flag, host's children need to be lazy reattached in adding a shadow root, which would be done in resolve assignments if we set a distribution recalc flag. This optimization would make loading slightly faster when there are many shadow trees which will not have a slot because we do not need to traverse down the trees. In the case of bug 699838, this can make loading 5ms (at most) faster. [1] https://codereview.chromium.org/1995203002 BUG=712559 Review-Url: https://codereview.chromium.org/2822113002 Cr-Commit-Position: refs/heads/master@{#465547} Committed: https://chromium.googlesource.com/chromium/src/+/a9c70fe4e42df88f817cb00cbc13... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a9c70fe4e42df88f817cb00cbc13...
Message was sent while issue was closed.
On 2017/04/19 10:35:27, hayato wrote: > On 2017/04/18 at 08:24:33, kochi wrote: > > LGTM > > > > Below is more of a question than a comment: > > > > Even without this change, when no slot is in the V1 shadow > > root, can SlotAssignment::ResolveDistribution() be almost > > no-op? > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/shado... > > > > ResolveDistribution() calls ResolveAssignment(), > > which calls DetachNotAssignedNode() for all children > > of shadow host. Is the time saved with this change > > mostly spent in DetachNotAssignedNode() then?? > > No. What can be saved are the *number* of calling ResolveAssignment and tree > traversing cost. > RecalcDistribution is a recursive operation. We have to traverse every shadow > trees, calling RessolveDistribution N times (N == the number of custom > elements). I see, thanks for the explanation! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
