Make customElements.define faster

This cuts the overhead of customElements.define by roughly half, by:

- Keeping custom element callbacks alive with wrapper tracing, not maps.
  Reading and writing maps is apparently slow.

- Looking up the definition for a constructor with constructor, private
  symbol, array index; instead of private symbol to retrieve a map, map
  lookup, string marshal, hash lookup.

- Retrieving callbacks uses one handle scope instead of creating one per
  retrieval.

- Constructing a definition moves the observed attributes hash to the
  definition instead of copying it.

This does not change behavior.

BUG=710184

[dominicc (has gone to gerrit), 2017-04-19 07:52:29]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 07:52:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2017-04-19 07:55:28]

dominicc@chromium.org changed reviewers:
+ haraken@chromium.org, mlippautz@chromium.org

[dominicc (has gone to gerrit), 2017-04-19 08:01:03]

This is a bit messy WIP but early feedback appreciated. Getting rid of the map
manipulation takes this benchmark from Polymer:

https://jsbin.com/lokecuriyi/edit?js,output

from spending 3200 ticks in V8CustomElementRegistry::defineMethodCallback to
spending 1200 ticks there.

I haven't measured the effect on memory, advice welcome.

[commit-bot: I haz the power, 2017-04-19 08:16:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 08:16:03]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[dominicc (has gone to gerrit), 2017-04-19 08:20:04]

On 2017/04/19 at 08:01:03, dominicc wrote:
> This is a bit messy WIP but early feedback appreciated. Getting rid of the map
manipulation takes this benchmark from Polymer:
> 
> https://jsbin.com/lokecuriyi/edit?js,output
> 
> from spending 3200 ticks in V8CustomElementRegistry::defineMethodCallback to
spending 1200 ticks there.
> 
> I haven't measured the effect on memory, advice welcome.

* Correction--1600 ticks. It turns out to be roughly half the time.

[dominicc (has gone to gerrit), 2017-04-19 08:22:57]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 08:23:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Lippautz, 2017-04-19 08:49:12]

Looked at the tracing-related parts.

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:158:
visitor->TraceWrappers(connected_callback_);
Continued from the declaration: 
  visitor->TraceWrappers(connected_callback.Cast<v8::Value>());

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:283:
return constructor_.NewLocal(script_state_->GetIsolate()).As<v8::Object>();
This cast is not needed then. 

(And all below.)

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h
(right):

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:96:
TraceWrapperV8Reference<v8::Value> constructor_;
The intention of the template was that you can keep your specific types here and
use e.g. connected_callback.Cast<v8::Value>() in the tracing function. (see
below).

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md
(right):

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md:24:
``visitor->TraceWrappers(<m_field>)`` in the body of ``DEFINE_TRACE_WRAPPERS``.
Oh boy :)

Thanks, we could also do this in a separate CL if you like.

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:28: public
TraceWrapperBase {
I don't see why this one should be traced. 

TraceWrapperBase means that the class itself doesn't have a wrapper assigned.
The trace wrappers method below also doesn't trace any children. 

From a GC perspective this doesn't keep alive anything.

[haraken, 2017-04-19 09:20:57]

Using the wrapper tracing looks nice, but would you help me understand why this
CL improves performance? It's still reading/writing maps in
CustomElementRegistry::define.

[commit-bot: I haz the power, 2017-04-19 10:16:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 10:16:19]

Dry run: This issue passed the CQ dry run.

[dominicc (has gone to gerrit), 2017-04-20 01:01:19]

On 2017/04/19 at 09:20:57, haraken wrote:
> Using the wrapper tracing looks nice, but would you help me understand why
this CL improves performance? It's still reading/writing maps in
CustomElementRegistry::define.

See the diff in ScriptCustomElementDefinition.cpp; by 'map' I mean v8::Map.
Instead of reading a private property and setting 1-5 v8::Map entries it sets
one private property.

[dominicc (has gone to gerrit), 2017-04-20 01:05:38]

On 2017/04/19 at 08:49:12, mlippautz wrote:
> Looked at the tracing-related parts.
> 
>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):
> 
>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:158:
visitor->TraceWrappers(connected_callback_);
> Continued from the declaration: 
>   visitor->TraceWrappers(connected_callback.Cast<v8::Value>());
> 
>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:283:
return constructor_.NewLocal(script_state_->GetIsolate()).As<v8::Object>();
> This cast is not needed then. 
> 
> (And all below.)

Awesome thanks for the pointer. So the thing doing the tracing guarantees
there's a handle scope for these?

>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h
(right):
> 
>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:96:
TraceWrapperV8Reference<v8::Value> constructor_;
> The intention of the template was that you can keep your specific types here
and use e.g. connected_callback.Cast<v8::Value>() in the tracing function. (see
below).
> 
>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md
(right):
> 
>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md:24:
``visitor->TraceWrappers(<m_field>)`` in the body of ``DEFINE_TRACE_WRAPPERS``.
> Oh boy :)
> 
> Thanks, we could also do this in a separate CL if you like.
> 
>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):
> 
>
https://codereview.chromium.org/2828643002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:28: public
TraceWrapperBase {
> I don't see why this one should be traced. 
> 
> TraceWrapperBase means that the class itself doesn't have a wrapper assigned.
The trace wrappers method below also doesn't trace any children. 
> 
> From a GC perspective this doesn't keep alive anything.

This is here for the virtual dispatch so CustomElementRegistry which is the
wrappable can trace definitions without knowing explicitly about
ScriptCustomElementDefinition. Does that make sense?

[dominicc (has gone to gerrit), 2017-04-27 09:22:33]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 09:22:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2017-04-27 09:25:27]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 09:25:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 10:59:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 10:59:55]

Dry run: This issue passed the CQ dry run.

[dominicc (has gone to gerrit), 2017-04-28 02:36:40]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 02:37:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2017-04-28 02:40:02]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 02:41:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2017-04-28 02:46:22]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 02:46:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2017-04-28 02:49:59]

Description was changed from

==========
Keep custom element callbacks alive with wrapper tracing, not maps.

Reading and writing maps is apparently slow, so this makes
customElements.define more responsive.

BUG=710184
==========

to

==========
This cuts the overhead of customElements.define by roughly half, by:

- Keeping custom element callbacks alive with wrapper tracing, not maps.
  Reading and writing maps is apparently slow.

- Looking up the definition for a constructor with constructor, private
  symbol, array index; instead of private symbol to retrieve a map, map
  lookup, string marshal, hash lookup.

- Retrieving callbacks uses one handle scope instead of creating one per
  retrieval.

- Constructing a definition moves the observed attributes hash to the
  definition instead of copying it.

This does not change behavior.

BUG=710184
==========

[dominicc (has gone to gerrit), 2017-04-28 04:35:35]

dominicc@chromium.org changed reviewers:
+ kochi@chromium.org

[dominicc (has gone to gerrit), 2017-04-28 04:35:35]

PTAL

kochi: core/dom/custom
haraken, mlippautz: Wrapper tracing
blink-reviews-bindings: Bindings

[kochi, 2017-04-28 06:24:16]

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp
(right):

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp:266:
CustomElementDefinition::Id id) const {
Add DCHECK(id <= difinitions_.size())?

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.h (right):

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.h:57:
CustomElementDefinition* DefinitionForId(CustomElementDefinition::Id) const;
Make this private.

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.h:93:
DefinitionMap nameIdMap_;
s/nameIdMap_/name_id_map_/

[commit-bot: I haz the power, 2017-04-28 06:33:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 06:33:27]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-28 08:49:30]

It looks like this CL is making a lot of changes in one go. Maybe can we split
the CL into a couple of pieces?

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:134:
visitor->TraceWrappers(constructor_.Cast<v8::Value>());

Won't visitor->TraceWrappers(constructor_) work?

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:70:
return script_state_->ContextIsValid();

What is this change for?

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:76:
v8::TryCatch try_catch(isolate);

Are you intentionally moving the TryCatch scope to the caller sites?

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:9: #include
"bindings/core/v8/ScriptWrappable.h"  // For TraceWrapperBase

We normally don't add this kind of comment.

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/bindings/V8PerContextData.h (right):

https://codereview.chromium.org/2828643002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/bindings/V8PerContextData.h:152: // to its
definition.

Can we use V8PrivateProperty instead?