First implementation of lazily cached accessor for DOM attributes.

First implementation of lazily cached accessor for DOM attributes.

We use a generated callback to populate private property in the object on first
access in v8. The generated callback is also used when setting a new value for
the property. The CachedAccessor = Lazy tag is used (currently) for
HTMLElement.title, HTMLElement.lang and Element.id. This is just as a proof of
concept. More attributes could be converted.

BUG=chromium:667237

[nverne, 2017-05-09 02:07:48]

Description was changed from

==========
First implementation of lazily cached accessor for DOM attributes.

We use a generated callback to populate private property in the object on first
access in v8. The generated callback is also used when setting a new value for
the property. The CachedAccessor = Lazy tag is used (currently) for
HTMLElement.title, HTMLElement.lang and Element.id. This is just as a proof of
concept. More attributes could be converted.

BUG=
==========

to

==========
First implementation of lazily cached accessor for DOM attributes.

We use a generated callback to populate private property in the object on first
access in v8. The generated callback is also used when setting a new value for
the property. The CachedAccessor = Lazy tag is used (currently) for
HTMLElement.title, HTMLElement.lang and Element.id. This is just as a proof of
concept. More attributes could be converted.

BUG=chromium:667237
==========

[nverne, 2017-05-09 02:11:23]

nverne@chromium.org changed reviewers:
+ haraken@chromium.org, sashab@chromium.org, slangley@chromium.org

[nverne, 2017-05-09 02:11:24]

This isn't ready to submit until the related v8 cl is accepted. I'm sending this
out for a look. All the
third_party/WebKit/Source/bindings/tests/results/core/... files are
auto-generated by run-bindings-tests --reset-results.

[haraken, 2017-05-09 02:27:42]

haraken@chromium.org changed reviewers:
+ yukishiino@chromium.org

[haraken, 2017-05-09 02:27:43]

+yukishiino for the usage of private properties.

[Yuki, 2017-05-09 06:06:31]

Overall l-g-t-m, but I'm wondering the following two points.

a) Can we make the lazy version default?
i.e. [CachedAccessor] => lazy, [CachedAccessor=NotLazy] => not lazy.
Because I think the lazy version will be used more often than non-lazy one.

b) Do we need the non-lazy version at all?
Window.document is only the use case of non-lazy version so far, and if we can
make it lazy, we don't need to support non-lazy version at all.

I'm concerned that, if we were going to introduce the lazy version in addition
to the non-lazy version, then the size of
V8DOMConfiguration::AccessorConfiguration increases by 8 bytes, which would be
an unignorable regression of the binary size.

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:264: auto prop
= V8PrivateProperty::Get{{attribute.cached_accessor_name}}(isolate);
We may want to use V8PrivateProperty::GetSymbol(isolate,
"{{interface_name}}#{{attribute.cached_accessor_name}}").  This is slower than
Get{{attribute.cached_accessor_name}}(), but CacheInitCallback should be rarely
called and it should be okay, I think.

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:267:
//LOG(ERROR) << "{{attribute.name}}CacheInitCallback";
nit: Remove this debugging code.

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:426: 
nit: Remove this empty line.

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:488: {% macro
attribute_accessor_configuration(attribute, is_attribute) %}
nit: s/is_attribute/is_data_property/

IDL attributes are defined to be ES accessor properties in Web IDL spec, but
Blink is using ES data properties for some limited cases.

"attribute" shouldn't mean "data property", so let's call it "data property".

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:529: {% set
config_pre = {
nit: I'd prefer naming this config_world_dependent or world_dependent_config.

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:535: "other" :
[
nit: "non-main" or something like that?
"non-main" is more specific and clearer than "other", I think.

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:542: {% set
accessor_only_fields = [] if is_attribute else [
nit: Can we fold this into config_post and rename it to
config_world_independent?

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:554: {% if
attribute.is_per_world_bindings %}
Great improvement!  Can we go farther?

I'm wondering if we can define a constexpr function that takes world-dependent
config first and world-independent config last (or vice-versa) and return the
struct that we want, in order to make this code cleaner.

I vaguely think whether we can do something like the following.

  constexpr V8DOMConfiguration::AccessorConfiguration
  V8DOMConfiguration::BuildAccessorConfigration(...);

  V8DOMConfiguration::AccessorConfiguration accessor_config[] = {
    V8DOMConfiguration::BuildAccessorConfiguration(
      *{{world_dependent_config}}, *{{world_independent_config}}),
    ...,
  };

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:573: {% set
is_attribute = true %}
nit: s/is_attribute/is_data_property/

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:579: {% set
is_attribute = false %}
ditto

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:943: 
nit: Remove this empty line.

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLElement.idl (right):

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLElement.idl:25: [CEReactions, Reflect,
CachedAccessor=Lazy] attribute DOMString title;
Just curious, are |title| and |lang| so important?

I understand |id| is quite often used and worth using the cached accessor.  But
I didn't imagine that |title| and |lang| are heavily used.

[nverne, 2017-05-10 02:11:49]

I'm getting pushback from v8 team about using the cache_init_callback at all.
This CL may change substantially depending on that work. 

I think making lazy caching into the default is probably right. I'll do that in
a follow up change.

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:267:
//LOG(ERROR) << "{{attribute.name}}CacheInitCallback";
On 2017/05/09 at 06:06:31, Yuki(ooo_til_may8) wrote:
> nit: Remove this debugging code.

Done

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:426: 
On 2017/05/09 at 06:06:31, Yuki(ooo_til_may8) wrote:
> nit: Remove this empty line.

Done

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:488: {% macro
attribute_accessor_configuration(attribute, is_attribute) %}
On 2017/05/09 at 06:06:31, Yuki(ooo_til_may8) wrote:
> nit: s/is_attribute/is_data_property/
> 
> IDL attributes are defined to be ES accessor properties in Web IDL spec, but
Blink is using ES data properties for some limited cases.
> 
> "attribute" shouldn't mean "data property", so let's call it "data property".

This macro builds either an AccessorConfiguration or an AttributeConfiguration
from a python attribute object.  The meaning we want is now captured in the two
macros attribute_configuration() and accessor_configuration() calling this one.
Previously we only had the former macro and it forced us to include the same
fields in the underlying C++ structs, but this change allows them to safely
diverge. 

How about if I rename the macro build_configuration_from_attribute() and have
the change the second parameter to a string specifying the type of config?

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:943: 
On 2017/05/09 at 06:06:31, Yuki(ooo_til_may8) wrote:
> nit: Remove this empty line.

Done

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLElement.idl (right):

https://codereview.chromium.org/2873543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLElement.idl:25: [CEReactions, Reflect,
CachedAccessor=Lazy] attribute DOMString title;
On 2017/05/09 at 06:06:31, Yuki(ooo_til_may8) wrote:
> Just curious, are |title| and |lang| so important?
> 
> I understand |id| is quite often used and worth using the cached accessor. 
But I didn't imagine that |title| and |lang| are heavily used.

haraken@ said these were good test cases for the mechanism itself. This plan is
to migrate lots of DOM properties to lazy caching, and these three are a
reasonable start. We can always revisit the choices of which attributes deserve
caching in future.

[peria, 2017-05-10 02:21:52]

peria@chromium.org changed reviewers:
+ peria@chromium.org

[peria, 2017-05-10 02:21:53]

Could you split the refactoring in templates to another CL?
The refactoring is great, but most changes in it are
not critical for the issue you're addressing.

[nverne, 2017-05-23 05:51:37]

On 2017/05/10 at 02:21:53, peria wrote:
> Could you split the refactoring in templates to another CL?
> The refactoring is great, but most changes in it are
> not critical for the issue you're addressing.

I'm closing this issue for now, because the v8 companion of this issue is no
longer implemented in terms of a cache_init_callback.