New issue
Advanced search Search tips

Issue 906494 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Make flat tree traversal faster (for host's children)

Project Member Reported by hayato@chromium.org, Nov 19

Issue description

Flat tree traversal for host's children wouldn't be cheap.

For example, in case of nextSibling:

1. It needs to lookup node's slotname to its assigned slot in shadow root's slot assignment hashmap
2. It needs to search an index of the given node in the slot's assigned nodes, or lookup an index in node-to-index-map
   1. If we always use hashmap here, we always have to pay the cost of building hashmap even if there are only the small number of assigned nodes, where a simple liner search is much faster.


My idea to improvement is:

Let node have additional pointers to flat tree's parent, previous, next, to make flat tree traversal of host's children faster.
These pointers are only used for host's children. So this shouldn't add any cost for other cases.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2277a1c525c6cac5cfb065a71fbb625adf5dd53

commit d2277a1c525c6cac5cfb065a71fbb625adf5dd53
Author: Hayato Ito <hayato@chromium.org>
Date: Mon Nov 19 09:35:30 2018

Make flat tree traversal faster

Current implementation of flat tree traversal is not cheap when traversing host's children.
See http://crbug.com/906494 for details.

There was an attempt to make it faster in v1, https://crrev.com/c/1328622, however, this idea was
already implemented in v0, and this attempt actually caused a perf regression in some cases
( https://crbug.com/905197 ).

This CL makes flat tree traversal for host's children much faster in a different approach.
The basic idea is:

1. Let NodeRareData has additional pointers, as FlatTreeNodeData, and set pointers at the timing of
   slot assignment recalc.
2. At flat tree traversal, use these pointers to skip hashmap lookup.

In this CL, |next| and |previous| pointers for assigned nodes are used in flat tree traversal.
Thanks to that, flat tree traversal no longer lookup node-to-index hashmap, nor do a liner search
there.

These additional pointers are only set for host's children. There shouldn't be additional cost
for other cases.

In a follow-up CL, I'll use FlatTreeNodeData's |assigned_slot| to make flat tree traversal and
other operations much faster again. Since this task might be complicated, that can be done in
another CL.

BUG: 906494

Change-Id: I617da70371d6733050b7dc754d3b40c88c090c79
Reviewed-on: https://chromium-review.googlesource.com/c/1337225
Commit-Queue: Hayato Ito <hayato@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609212}
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/BUILD.gn
[add] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/flat_tree_node_data.cc
[add] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/flat_tree_node_data.h
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/flat_tree_traversal.cc
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/node.cc
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/node.h
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/node_rare_data.cc
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/node_rare_data.h
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/dom/slot_assignment.cc
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/html/html_slot_element.cc
[modify] https://crrev.com/d2277a1c525c6cac5cfb065a71fbb625adf5dd53/third_party/blink/renderer/core/html/html_slot_element.h

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4377df6fcb5e5749cd359bf11656ef1ea53e99c6

commit 4377df6fcb5e5749cd359bf11656ef1ea53e99c6
Author: Hayato Ito <hayato@chromium.org>
Date: Tue Nov 20 09:31:39 2018

Make flat tree traversal faster (2nd wave)

The 1st wave is here: https://crrev.com/c/1337225.

This CL makes flat tree traversal faster, again, by utilizing FlatTreeNodeData::assigned_slot_
field. This field is now set at the timing of reclc slot assignement. This filed is set to nullptr
if host's child is not assigned to any slot there.

In FlatTreeTraversal::TraverseSiblingsForV1HostChild, we check this field to avoid hashmap lookup
there.

BUG=906494

Change-Id: I4d5133821de1de2e58d08c7a31a7853017fd67ba
Reviewed-on: https://chromium-review.googlesource.com/c/1341744
Commit-Queue: Hayato Ito <hayato@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609644}
[modify] https://crrev.com/4377df6fcb5e5749cd359bf11656ef1ea53e99c6/third_party/blink/renderer/core/dom/flat_tree_node_data.h
[modify] https://crrev.com/4377df6fcb5e5749cd359bf11656ef1ea53e99c6/third_party/blink/renderer/core/dom/flat_tree_traversal.cc
[modify] https://crrev.com/4377df6fcb5e5749cd359bf11656ef1ea53e99c6/third_party/blink/renderer/core/dom/node.cc
[modify] https://crrev.com/4377df6fcb5e5749cd359bf11656ef1ea53e99c6/third_party/blink/renderer/core/dom/node.h
[modify] https://crrev.com/4377df6fcb5e5749cd359bf11656ef1ea53e99c6/third_party/blink/renderer/core/dom/shadow_root.h
[modify] https://crrev.com/4377df6fcb5e5749cd359bf11656ef1ea53e99c6/third_party/blink/renderer/core/dom/slot_assignment.cc
[modify] https://crrev.com/4377df6fcb5e5749cd359bf11656ef1ea53e99c6/third_party/blink/renderer/core/html/html_slot_element.cc
[modify] https://crrev.com/4377df6fcb5e5749cd359bf11656ef1ea53e99c6/third_party/blink/renderer/core/html/html_slot_element.h

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9cd83640849a69c50de2c4d8727cb9835dcee463

commit 9cd83640849a69c50de2c4d8727cb9835dcee463
Author: Hayato Ito <hayato@chromium.org>
Date: Thu Nov 29 08:24:47 2018

Disallow recursive recalc slot assignment

This CL is separated out from the wip CL, https://crrev.com/c/1350441,
where recursive recalc slot assignment causes the tricky bug which is
hard to debug.

It would be better to detect recursive recalc slot assignment, which
shouldn't happen.

Change-Id: I07d2d22ea8e083ef1de5134d6879cb1a141ce183

BUG=906494

Change-Id: I07d2d22ea8e083ef1de5134d6879cb1a141ce183
Reviewed-on: https://chromium-review.googlesource.com/c/1354727
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Hayato Ito <hayato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612098}
[modify] https://crrev.com/9cd83640849a69c50de2c4d8727cb9835dcee463/third_party/blink/renderer/core/dom/slot_assignment.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9cba271a2ad1761f12731887bb4427fa7719da2

commit d9cba271a2ad1761f12731887bb4427fa7719da2
Author: Hayato Ito <hayato@chromium.org>
Date: Fri Nov 30 03:44:24 2018

Guard fast flat tree traversal (fftt) by runtime flag

While in working on the WIP CL, https://crrev.com/c/1350441, I've found that
there is an edge case where fast flat tree traversal might return dirty
result. So it would be safe to guard the feature of fast flat tree traversal
by the runtime flag, and continue to develop under the flag.

This CL is basically rewriting the following two CLs with runtime flag,

- https://crrev.com/c/1337225
- https://crrev.com/c/1341744

and bring back old code which runs when the flag is disabled.

BUG: 906494

Change-Id: I7a1940643925c1ef583cf55f1a79bfa0190ef40c
Reviewed-on: https://chromium-review.googlesource.com/c/1356167
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Hayato Ito <hayato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612548}
[modify] https://crrev.com/d9cba271a2ad1761f12731887bb4427fa7719da2/third_party/blink/renderer/core/dom/flat_tree_traversal.cc
[modify] https://crrev.com/d9cba271a2ad1761f12731887bb4427fa7719da2/third_party/blink/renderer/core/dom/slot_assignment.cc
[modify] https://crrev.com/d9cba271a2ad1761f12731887bb4427fa7719da2/third_party/blink/renderer/core/html/html_slot_element.cc
[modify] https://crrev.com/d9cba271a2ad1761f12731887bb4427fa7719da2/third_party/blink/renderer/core/html/html_slot_element.h
[modify] https://crrev.com/d9cba271a2ad1761f12731887bb4427fa7719da2/third_party/blink/renderer/platform/runtime_enabled_features.json5

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f312bb2eb90f51f2d79933aa4836fa32bc75de5c

commit f312bb2eb90f51f2d79933aa4836fa32bc75de5c
Author: Hayato Ito <hayato@chromium.org>
Date: Mon Dec 03 11:53:50 2018

[Fast flat tree traversal] Make AssignedSlot faster

Make flat tree traversal's parent operation faster by avoiding
a lookup of a hashmap (slotname-to-slot) (in ShadowRoot::AssignedSlotFor)
if FlatTreeNodeData's assigned_slot_ can be used.

The feature is under the FastFlatTreeTraversal flag.

BUG=906494

Change-Id: Ia93b8a045146cdfb7008fc5e5914f19ecbd0fdc6
Reviewed-on: https://chromium-review.googlesource.com/c/1350441
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Hayato Ito <hayato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613052}
[modify] https://crrev.com/f312bb2eb90f51f2d79933aa4836fa32bc75de5c/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/f312bb2eb90f51f2d79933aa4836fa32bc75de5c/third_party/blink/renderer/core/dom/flat_tree_node_data.h
[modify] https://crrev.com/f312bb2eb90f51f2d79933aa4836fa32bc75de5c/third_party/blink/renderer/core/dom/node.cc
[modify] https://crrev.com/f312bb2eb90f51f2d79933aa4836fa32bc75de5c/third_party/blink/renderer/core/dom/slot_assignment.cc

Sign in to add a comment