New issue
Advanced search Search tips

Issue 764618 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 705187
issue 771487



Sign in to add a comment

Umbrella bug for micro-optimizations in appendChild/removeChild

Project Member Reported by jbroman@chromium.org, Sep 13 2017

Issue description

While visiting TOK, I've been profiling a benchmark that appends and removes children (which is in many ways also similar to parserAppendChild). This bug captures the variety of little things arising from that.

It's fun to note that about 2% of appendChildMethodCallbackForMainWorld is the bit that actually updates the pointers, and the rest is mostly notifying various parts of the DOM system about the possibility of an interesting child being added.
 
WIP patches:

HTMLFrameOwnerElement::PluginDisposeSuspendScope (about 3% of removeChild). In the overwhelmingly common case of no plugins being disposed, we can inline almost everything into a few instructions.
https://chromium-review.googlesource.com/c/chromium/src/+/663299

Reduce memory loads while comparing objects to FunctionTemplate, especially where walk is required (i.e. for V8Node::HasInstance).
https://chromium-review.googlesource.com/c/v8/v8/+/662380

V8FunctionTemplateMap lookup spends significant time in hash computation, but a full PtrHash is overkill here, since these are all static addresses anyhow.
https://chromium-review.googlesource.com/c/chromium/src/+/663278

Optimize live node lists in Document
https://chromium-review.googlesource.com/c/chromium/src/+/664261
Blocking: 705187
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 14 2017

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

commit 2700afca42099e3af0c2eed7a1fef7f1559c55d3
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Sep 14 02:46:48 2017

Make HTMLFrameOwnerElement::PluginDisposeSuspendScope more lightweight.

This allows nearly all of the logic to be inline and only a few
instructions long. The set itself is also made a persistent collection,
as otherwise DEFINE_STATIC_LOCAL will wrap it in a Persistent<> handle,
which is an unnecessary indirection here.

Virtually all of the time, there is no suspended plugin disposal in a
child node removal.

Saves about 3% in a microbenchmark that does appendChild/removeChild
in a loop.

Bug: 764618
Change-Id: I231b8c769d801c461482a129345517303baf657c
Reviewed-on: https://chromium-review.googlesource.com/663299
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501850}
[modify] https://crrev.com/2700afca42099e3af0c2eed7a1fef7f1559c55d3/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp
[modify] https://crrev.com/2700afca42099e3af0c2eed7a1fef7f1559c55d3/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 14 2017

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

commit 088e833442d66036b2696ee6903440ef75402dac
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Sep 14 12:08:53 2017

Use a trivial hash function for V8FunctionTemplateMap.

These maps are populated with static pointers, and so don't need to be
as well-mixed as some other hash maps. Surprisingly, hashing the pointer
to lookup is quite significant. This is a much simpler hash function
that chops off the low (alignment bits), and then xors some of it back
in (as there are a few ints that the compiler might choose to pack).

In practice this is good enough to avoid pathological collisions, and
this makes an appendChild/removeChild benchmark about 10% faster,
purely by making the Node argument conversion lookup cheaper.

Bug: 764618
Change-Id: I78534f9a3c16da5001512add36aabf49022cc04e
Reviewed-on: https://chromium-review.googlesource.com/663278
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501927}
[modify] https://crrev.com/088e833442d66036b2696ee6903440ef75402dac/third_party/WebKit/Source/platform/bindings/V8PerIsolateData.h

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 2 2017

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

commit 2e63fde900b233194ce1ef52acf160f0b410131e
Author: Jeremy Roman <jbroman@chromium.org>
Date: Mon Oct 02 19:32:09 2017

Make Document-level tracking of live node lists faster.

This is very hot (because it is queried in every appendChild, including
during parsing). Consequently, this CL implements a compact collection
that weakly holds live node lists and updates a bit mask indicating
what kind of lists are present as it is modified. Querying this, e.g.
to tell if we need to invalidate during child list modification, then
becomes a single load and a bitwise-and.

The mask needs to be recomputed only if a node list owner is adopted
by another document (rare) or when one or more such lists are collected
(also rare, generally not in the hot path, and comparable to what
HeapHashSet<WeakMember<>> does).

Saves about 10% in a micro-benchmark of adding/removing child nodes from
script (per my local testing).

Bug: 764618
Change-Id: Iba1dd254c145df3dc00c622907d53856ceae32e2
Reviewed-on: https://chromium-review.googlesource.com/664261
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505721}
[modify] https://crrev.com/2e63fde900b233194ce1ef52acf160f0b410131e/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/2e63fde900b233194ce1ef52acf160f0b410131e/third_party/WebKit/Source/core/dom/BUILD.gn
[modify] https://crrev.com/2e63fde900b233194ce1ef52acf160f0b410131e/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/2e63fde900b233194ce1ef52acf160f0b410131e/third_party/WebKit/Source/core/dom/Document.h
[add] https://crrev.com/2e63fde900b233194ce1ef52acf160f0b410131e/third_party/WebKit/Source/core/dom/LiveNodeListRegistry.cpp
[add] https://crrev.com/2e63fde900b233194ce1ef52acf160f0b410131e/third_party/WebKit/Source/core/dom/LiveNodeListRegistry.h
[add] https://crrev.com/2e63fde900b233194ce1ef52acf160f0b410131e/third_party/WebKit/Source/core/dom/LiveNodeListRegistryTest.cpp
[modify] https://crrev.com/2e63fde900b233194ce1ef52acf160f0b410131e/third_party/WebKit/Source/core/dom/NameNodeList.h

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 2 2017

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

commit 1c23d6ebbe6f1dfb85fe65f746ef1ebd1292706b
Author: Adithya Srinivasan <adithyas@chromium.org>
Date: Mon Oct 02 21:45:51 2017

Move attribute check in Element::insertedInto to the end

The attribute check is the slowest check in that if statement, and it's
likely for some of the other checks to fail first.

Bug: 764618
Change-Id: I6dd694f9ace8fc5e9461a51f6eab8abdec32580b
Reviewed-on: https://chromium-review.googlesource.com/695982
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505798}
[modify] https://crrev.com/1c23d6ebbe6f1dfb85fe65f746ef1ebd1292706b/third_party/WebKit/Source/core/html/HTMLElement.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 4 2017

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

commit 1668906267724e46feb46065be4ea48b2fcfdbfc
Author: Jeremy Roman <jbroman@chromium.org>
Date: Wed Oct 04 00:23:34 2017

probe: Fast return path when no inspector agents exist.

This allows probes that are dispatched to some set of agent types to
rapidly query (by masking the bits of a global bit-set) the existence
of any matching agent enabled in the renderer process. This bypasses
the logic needed to locate and search the probe sink.

This check is very small and usually fails, so it is provided inline
in the header (and the remaining function is renamed to probe::fooImpl).

This bit-set is maintained by sinks which count the number of sinks
which have such an agent (in add, remove and destructor), protected by
a mutex, and atomically updating the global state when the first agent
of a type is added or the last is removed.

Finally, templates are amended to properly include the export header,
rather than relying on it already having been included in the
translation unit.

This yields a 12% improvement on a appendChild/removeChild
microbenchmark (as measured on a Linux workstation).

Bug: 764618
Change-Id: I9f54ae349395f58bf336afe0714dff2eb63aab03
Reviewed-on: https://chromium-review.googlesource.com/685644
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506233}
[modify] https://crrev.com/1668906267724e46feb46065be4ea48b2fcfdbfc/third_party/WebKit/Source/build/scripts/templates/InstrumentingProbesImpl.cpp.tmpl
[modify] https://crrev.com/1668906267724e46feb46065be4ea48b2fcfdbfc/third_party/WebKit/Source/build/scripts/templates/InstrumentingProbesInl.h.tmpl
[modify] https://crrev.com/1668906267724e46feb46065be4ea48b2fcfdbfc/third_party/WebKit/Source/build/scripts/templates/ProbeSink.h.tmpl
[modify] https://crrev.com/1668906267724e46feb46065be4ea48b2fcfdbfc/third_party/WebKit/Source/core/probe/CoreProbes.json5
[modify] https://crrev.com/1668906267724e46feb46065be4ea48b2fcfdbfc/third_party/WebKit/Source/platform/probe/PlatformProbes.json5

Blocking: 771487
Forgot to link this one:

https://chromium.googlesource.com/chromium/src/+/55f0aee423f725c68c76f3f5d2017e63d2a0f3c5

Move InputMethodController, SelectionController and IdleSpellCheckCallback to DocumentShutdownObserver.

They do not observe any other kind of change to the document, and so can only
register for destruction notification.

This saves 5% on an appendChild/removeChild microbenchmark (Linux workstation).

Change-Id: I0cc469ae2aff9786a830f717ba9259d0bd132ad9
Reviewed-on: https://chromium-review.googlesource.com/701794
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506725}
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 11 2017

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

commit 2c9daef5c13f98b195315821c4832437e2c5c2fe
Author: Jeremy Roman <jbroman@chromium.org>
Date: Wed Oct 11 16:18:08 2017

DocumentMarkerController: Only watch document mutation when markers may exist.

Since this increases the number of cases where the context is changed to its
existing value, make LifecycleObserver check for this case rather than
thrashing the hash map.

Bug: 764618
Change-Id: I7f736c44b81749a609390bc0a74aed95dd72a699
Reviewed-on: https://chromium-review.googlesource.com/702041
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507997}
[modify] https://crrev.com/2c9daef5c13f98b195315821c4832437e2c5c2fe/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/2c9daef5c13f98b195315821c4832437e2c5c2fe/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
[modify] https://crrev.com/2c9daef5c13f98b195315821c4832437e2c5c2fe/third_party/WebKit/Source/platform/LifecycleObserver.h

Sign in to add a comment