Add [CachedAccessor] attribute to cache (almost) constant accessors (window.document).

Add [CachedAccessor] attribute to cache (almost) constant accessors.

Calls to getter are translated into a cheap property load, eliminating the calling overhead. Other potential benefits could also apply, like inlining and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property for performance reasons, with the downside of violating the spec. This produces exactly the same (performant) code as the current hack while being spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);


To invalidate/update (e.g. window.document) just use:

V8PrivateProperty::getWindowDocument().set(context, window, newDocumentWrapper);

BUG=chromium:634276
Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ


Specialize GlobalProxy access:
https://codereview.chromium.org/2369933005

[Alfonso, 2016-09-15 07:53:35]

Description was changed from

==========
Add [StoreInField] attribute to cache (almost) constant accessors e.g. document.
Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

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

to

==========
Add [StoreInField] attribute to cache (almost) constant accessors e.g. document.
Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);

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

[Alfonso, 2016-09-15 07:54:11]

peterssen@google.com changed reviewers:
+ jochen@chromium.org, vogelheim@chromium.org

[Alfonso, 2016-09-15 07:56:52]

On 2016/09/15 07:54:11, Alfonso wrote:
> mailto:peterssen@google.com changed reviewers:
> + mailto:jochen@chromium.org, mailto:vogelheim@chromium.org

PTAL. This is the Blink-side of the CL.

[jochen (gone - plz use gerrit), 2016-09-15 07:58:07]

can you please file a tracking bug and reference it from the cl description
(under crbug.com/new)

[Alfonso, 2016-09-15 08:08:03]

Description was changed from

==========
Add [StoreInField] attribute to cache (almost) constant accessors e.g. document.
Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);

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

to

==========
Add [StoreInField] attribute to cache (almost) constant accessors e.g. document.
Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);

BUG=chromium:634276
==========

[Alfonso, 2016-09-15 08:09:55]

On 2016/09/15 07:58:07, jochen wrote:
> can you please file a tracking bug and reference it from the cl description
> (under crbug.com/new)

Reusing crbug.com/634276

[Yuki, 2016-09-15 08:17:33]

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

[Yuki, 2016-09-15 08:17:34]

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:416:
v8::Local<v8::Private> priv =
cachedAccessors().at("Window#document").Get(m_isolate);
Can you use V8PrivateProperty?

I think std::map and std::string are discouraged in Blink.

[jochen (gone - plz use gerrit), 2016-09-15 08:20:50]

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

[jochen (gone - plz use gerrit), 2016-09-15 08:20:51]

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1579: Summary:
Caches invariable accessors to a private property. Private property name is
automatically generated e.g. "Class#attribute".
hum, that sounds like there'd be some automatic caching and invalidation going
on, but that's not the case, no?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:131:
v8::Local<v8::Function>
createAccessorFunctionOrTemplate<v8::Function>(v8::Isolate* isolate,
v8::FunctionCallback callback, V8DOMConfiguration::FastAccessorBuilderCallback,
const char* const, v8::Local<v8::Value> data, v8::Local<v8::Signature>
signature, int length)
why not just const char*?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:136:
v8::Local<v8::FunctionTemplate> functionTemplate =
createAccessorFunctionOrTemplate<v8::FunctionTemplate>(isolate, callback,
nullptr, 0, data, signature, length);
nullptr

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:187:
instanceOrTemplate->Set(v8::Private::AsSymbol(property).As<v8::Name>(),
v8::Undefined(isolate));
undefined would be the wrong value here, shouldn't that be whatever the getter
returns?

Do layout tests pass with this?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:420:
CHECK(context->Global()->ForceSet(context, v8::Private::AsSymbol(priv),
documentWrapper).FromJust());
why forceset?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:93:
DCHECK(isMainThread());
that means that this feature is currently not available to workers, but since
it's on window proxy that's fine I guess

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:93:
DCHECK(isMainThread());
that means that this feature is currently not available to workers, but since
it's on window proxy that's fine I guess

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (left):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:122:
'has_fast_accessor': 'FastAccessor' in extended_attributes,
unrelated?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp:505: '0' %}
nullptr?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp:700:
"{{method.name}}", {{getter_callback}}, {{setter_callback}},
{{getter_callback_for_main_world}}, {{setter_callback_for_main_world}}, nullptr,
0, &{{v8_class}}::wrapperTypeInfo, v8::ALL_CAN_READ, {{property_attribute}},
{{only_exposed_to_private_script}}, {{property_location(method)}},
{{holder_check}},
nullptr?

[haraken, 2016-09-15 09:26:10]

This looks amazing!

I'm just curious but how are you planning to expose V8 APIs for cache
invalidation?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1577: ###
[StoreInField] _(a)_

[CacheAttributeGetterResult] ?

(Given that an extended attribute is a Blink thing, I want to name it from the
Blink perspective.)

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:109: const char*
const surrogatePropertyName; // The accessor is backed by a private property.

surrogatePropertyName => keyForCachedAttributeGetter ?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:94:
DEFINE_STATIC_LOCAL(StringPrivateMap, map, ());

If this map is intended to be per-isolate, can you put the map on
V8PerIsolateData? WindowProxy doesn't look like a right place to put it.

That said, as yukishiino@ mentioned, if you can use V8PrivateProperty, that will
be the best solution.

[vogelheim, 2016-09-15 15:29:47]

The naming is a bit inconsistent... surrogate + cache + StoreInField. We should
agree on a single name.. :)

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:110:
v8::String::NewFromUtf8(isolate, surrogateName,
v8::NewStringType::kNormal).ToLocalChecked();
nitpick: The line breaking is strange. Since Blink doesn't have an 80 columns
rule, maybe write it all in one line?

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:187:
instanceOrTemplate->Set(v8::Private::AsSymbol(property).As<v8::Name>(),
v8::Undefined(isolate));
On 2016/09/15 08:20:51, jochen wrote:
> undefined would be the wrong value here, shouldn't that be whatever the getter
> returns?
> 
> Do layout tests pass with this?

Wouldn't this usually be overwritten by WindowProxy::updateDocumentProperty()?

I'd think this would only be observable if one does the
new-window-then-navigate-away trick that forces this whole hack in the first
place. Not sure if this is tested, though. Anyway, would your suggestion be to
just call the getterCallback right here?

[vogelheim, 2016-09-15 16:06:35]

> That said, as yukishiino@ mentioned, if you can use V8PrivateProperty, that
will
> be the best solution.

We looked at this and didn't quite see how this would work... The design of
V8PrivateProperty is that id hides the actual v8::Local<v8::Private>, but we
need during registration of the accessor. We could add a getter to
V8PrivateProperty to expose the v8::Private, but that seems to violate the
design intent... Am I overlooking something?

[vogelheim, 2016-09-15 16:34:09]

On Thu, Sep 15, 2016 at 11:26 AM, <haraken@chromium.org> wrote:

> This looks amazing!
>
> I'm just curious but how are you planning to expose V8 APIs for cache
> invalidation?
>

We discussed several variants of this, but didn't reach a conclusion. So
there is presently no plan. :(

The main driver for proceeding with this CL as-is is that it solves two
immediate problems we're having: It fixes a spec violation for the document
accessor (which, currently, is a data property), and allows us to finally
remove the long-deprecated ForceSet method (which was only retained for the
current 'document hack').


Some incomplete thoughts on general cache invalidation:
- We rejected the idea of a check-validity callback, since that would ruin
the performance this is supposed to gain.
- Firefox has an interesting method where they can declare data
dependencies (e.g., depends on DOM node), which - I assume - their DOM
implementation knows about and will invalidate as needed. It's not clear to
me whether than can be adopted to Chromium; or whether we'd derive the same
benefit from it.
- The current implementation would probably allow implementation of any
accessor where the embedder pushes new updates, without further changes to
V8.
- I think your use case involves named properties, which would require at
least some additional work. Unfortunately, I'm a bit unsure on which parts
are missing.


Daniel

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[vogelheim, 2016-09-15 16:34:09]

On Thu, Sep 15, 2016 at 11:26 AM, <haraken@chromium.org> wrote:

> This looks amazing!
>
> I'm just curious but how are you planning to expose V8 APIs for cache
> invalidation?
>

We discussed several variants of this, but didn't reach a conclusion. So
there is presently no plan. :(

The main driver for proceeding with this CL as-is is that it solves two
immediate problems we're having: It fixes a spec violation for the document
accessor (which, currently, is a data property), and allows us to finally
remove the long-deprecated ForceSet method (which was only retained for the
current 'document hack').


Some incomplete thoughts on general cache invalidation:
- We rejected the idea of a check-validity callback, since that would ruin
the performance this is supposed to gain.
- Firefox has an interesting method where they can declare data
dependencies (e.g., depends on DOM node), which - I assume - their DOM
implementation knows about and will invalidate as needed. It's not clear to
me whether than can be adopted to Chromium; or whether we'd derive the same
benefit from it.
- The current implementation would probably allow implementation of any
accessor where the embedder pushes new updates, without further changes to
V8.
- I think your use case involves named properties, which would require at
least some additional work. Unfortunately, I'm a bit unsure on which parts
are missing.


Daniel

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2016-09-16 02:22:05]

On 2016/09/15 16:06:35, vogelheim wrote:
> > That said, as yukishiino@ mentioned, if you can use V8PrivateProperty, that
> will
> > be the best solution.
> 
> We looked at this and didn't quite see how this would work... The design of
> V8PrivateProperty is that id hides the actual v8::Local<v8::Private>, but we
> need during registration of the accessor. We could add a getter to
> V8PrivateProperty to expose the v8::Private, but that seems to violate the
> design intent... Am I overlooking something?

yukishiino@: Do you have any idea?

[haraken, 2016-09-16 02:27:23]

On 2016/09/15 16:34:09, vogelheim wrote:
> On Thu, Sep 15, 2016 at 11:26 AM, <mailto:haraken@chromium.org> wrote:
> 
> > This looks amazing!
> >
> > I'm just curious but how are you planning to expose V8 APIs for cache
> > invalidation?
> >
> 
> We discussed several variants of this, but didn't reach a conclusion. So
> there is presently no plan. :(

OK. Ideally I want to use this mechanism for all DOM attribute getters where the
attribute update is not common, so it's important to think about the
invalidation API :)

> 
> The main driver for proceeding with this CL as-is is that it solves two
> immediate problems we're having: It fixes a spec violation for the document
> accessor (which, currently, is a data property), and allows us to finally
> remove the long-deprecated ForceSet method (which was only retained for the
> current 'document hack').
> 
> 
> Some incomplete thoughts on general cache invalidation:
> - We rejected the idea of a check-validity callback, since that would ruin
> the performance this is supposed to gain.
> - Firefox has an interesting method where they can declare data
> dependencies (e.g., depends on DOM node), which - I assume - their DOM
> implementation knows about and will invalidate as needed. It's not clear to
> me whether than can be adopted to Chromium; or whether we'd derive the same
> benefit from it.
> - The current implementation would probably allow implementation of any
> accessor where the embedder pushes new updates, without further changes to
> V8.

I think that the push-based approach makes sense since the idea is to cache
results of DOM attribute getters where the attribute update is not common. For
other DOM attribute getters, we can just use a normal mechanism to call back the
getter.

> - I think your use case involves named properties, which would require at
> least some additional work. Unfortunately, I'm a bit unsure on which parts
> are missing.

We have decided not to add an interceptor for named properties on HTMLDocument.
So you can forget about the issue :)

[Alfonso, 2016-09-16 07:47:34]

On 2016/09/16 02:22:05, haraken wrote:
> On 2016/09/15 16:06:35, vogelheim wrote:
> > > That said, as yukishiino@ mentioned, if you can use V8PrivateProperty,
that
> > will
> > > be the best solution.
> > 
> > We looked at this and didn't quite see how this would work... The design of
> > V8PrivateProperty is that id hides the actual v8::Local<v8::Private>, but we
> > need during registration of the accessor. We could add a getter to
> > V8PrivateProperty to expose the v8::Private, but that seems to violate the
> > design intent... Am I overlooking something?
> 
> yukishiino@: Do you have any idea?

Using V8PrivateProperty would be ideal, everything static (no maps), per
isolate, invalidation/update for 'document' would be something like:

V8PrivateProperty::getWindowDocument().set(context, window, newDocumentWrapper);

We need access to the underlying m_privateSymbol. Otherwise we'll end up just
duplicating the same functionality.

[haraken, 2016-09-16 07:56:50]

On 2016/09/16 07:47:34, Alfonso wrote:
> On 2016/09/16 02:22:05, haraken wrote:
> > On 2016/09/15 16:06:35, vogelheim wrote:
> > > > That said, as yukishiino@ mentioned, if you can use V8PrivateProperty,
> that
> > > will
> > > > be the best solution.
> > > 
> > > We looked at this and didn't quite see how this would work... The design
of
> > > V8PrivateProperty is that id hides the actual v8::Local<v8::Private>, but
we
> > > need during registration of the accessor. We could add a getter to
> > > V8PrivateProperty to expose the v8::Private, but that seems to violate the
> > > design intent... Am I overlooking something?
> > 
> > yukishiino@: Do you have any idea?
> 
> Using V8PrivateProperty would be ideal, everything static (no maps), per
> isolate, invalidation/update for 'document' would be something like:
> 
> V8PrivateProperty::getWindowDocument().set(context, window,
newDocumentWrapper);
> 
> We need access to the underlying m_privateSymbol. Otherwise we'll end up just
> duplicating the same functionality.

Just to be clear: You need to access the underlying m_privateSymbol from V8 so
that V8 can access the document wrapper before calling back a C++ callback
(i.e., without leaving the JIT code), right?

[Alfonso, 2016-09-16 08:05:22]

On 2016/09/16 07:56:50, haraken wrote:
> On 2016/09/16 07:47:34, Alfonso wrote:
> > On 2016/09/16 02:22:05, haraken wrote:
> > > On 2016/09/15 16:06:35, vogelheim wrote:
> > > > > That said, as yukishiino@ mentioned, if you can use V8PrivateProperty,
> > that
> > > > will
> > > > > be the best solution.
> > > > 
> > > > We looked at this and didn't quite see how this would work... The design
> of
> > > > V8PrivateProperty is that id hides the actual v8::Local<v8::Private>,
but
> we
> > > > need during registration of the accessor. We could add a getter to
> > > > V8PrivateProperty to expose the v8::Private, but that seems to violate
the
> > > > design intent... Am I overlooking something?
> > > 
> > > yukishiino@: Do you have any idea?
> > 
> > Using V8PrivateProperty would be ideal, everything static (no maps), per
> > isolate, invalidation/update for 'document' would be something like:
> > 
> > V8PrivateProperty::getWindowDocument().set(context, window,
> newDocumentWrapper);
> > 
> > We need access to the underlying m_privateSymbol. Otherwise we'll end up
just
> > duplicating the same functionality.
> 
> Just to be clear: You need to access the underlying m_privateSymbol from V8 so
> that V8 can access the document wrapper before calling back a C++ callback
> (i.e., without leaving the JIT code), right?

Yes, being able to just read/write the property is not enough, V8 needs to know
where the value is located (the m_privateSymbol) in order to replace the call to
the getter with a property load.

[haraken, 2016-09-16 08:06:31]

On 2016/09/16 08:05:22, Alfonso wrote:
> On 2016/09/16 07:56:50, haraken wrote:
> > On 2016/09/16 07:47:34, Alfonso wrote:
> > > On 2016/09/16 02:22:05, haraken wrote:
> > > > On 2016/09/15 16:06:35, vogelheim wrote:
> > > > > > That said, as yukishiino@ mentioned, if you can use
V8PrivateProperty,
> > > that
> > > > > will
> > > > > > be the best solution.
> > > > > 
> > > > > We looked at this and didn't quite see how this would work... The
design
> > of
> > > > > V8PrivateProperty is that id hides the actual v8::Local<v8::Private>,
> but
> > we
> > > > > need during registration of the accessor. We could add a getter to
> > > > > V8PrivateProperty to expose the v8::Private, but that seems to violate
> the
> > > > > design intent... Am I overlooking something?
> > > > 
> > > > yukishiino@: Do you have any idea?
> > > 
> > > Using V8PrivateProperty would be ideal, everything static (no maps), per
> > > isolate, invalidation/update for 'document' would be something like:
> > > 
> > > V8PrivateProperty::getWindowDocument().set(context, window,
> > newDocumentWrapper);
> > > 
> > > We need access to the underlying m_privateSymbol. Otherwise we'll end up
> just
> > > duplicating the same functionality.
> > 
> > Just to be clear: You need to access the underlying m_privateSymbol from V8
so
> > that V8 can access the document wrapper before calling back a C++ callback
> > (i.e., without leaving the JIT code), right?
> 
> Yes, being able to just read/write the property is not enough, V8 needs to
know
> where the value is located (the m_privateSymbol) in order to replace the call
to
> the getter with a property load.

Makes sense. I'm fine with adding a method to expose
V8PrivateProperty::m_privateSymbol.

[Yuki, 2016-09-16 08:09:30]

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

[Yuki, 2016-09-16 08:09:31]

For WindowProxy::cachedAccessors(), IIUC, I don't see much benefit to cache
them.  Please correct me if I'm misunderstanding.

A v8::Private for a certain accessor property is created at
V8DOMConfiguration::createAccessorFunctionOrTemplate() and used (almost) only
once at V8DOMConfiguration::installAccessorInternal().  Because v8::Private is
per-isolate and v8::FunctionTemplate is also per-isolate, it's very rare to
reuse the v8::Private because 
we hardly re-install the same interface's same property again.  Seeing it's used
almost only once, is it worth caching them?  It seems to me we don't need this
cache at all.


For
V8DOMConfiguration::{Accessor,Attribute}Configuration::surrogatePropertyName, I
think we don't need this field.  When installing a property, we know its
interface name and property name, so we can create a string on the fly.  I think
we only need a single bit to specify that it should be
FunctionTemplate::NewSurrogate.

nit: I'd recommend "Surrogate#<interface_name>#<property_name>" rather than
"<interface_name>#<property_name>", which may conflict with other use cases of
v8::Private.

[Alfonso, 2016-09-16 08:46:20]

On 2016/09/16 08:09:31, Yuki wrote:
> For WindowProxy::cachedAccessors(), IIUC, I don't see much benefit to cache
> them.  Please correct me if I'm misunderstanding.
> 
> A v8::Private for a certain accessor property is created at
> V8DOMConfiguration::createAccessorFunctionOrTemplate() and used (almost) only
> once at V8DOMConfiguration::installAccessorInternal().  Because v8::Private is
> per-isolate and v8::FunctionTemplate is also per-isolate, it's very rare to
> reuse the v8::Private because 
> we hardly re-install the same interface's same property again.  Seeing it's
used
> almost only once, is it worth caching them?  It seems to me we don't need this
> cache at all.
> 
> 
> For
> V8DOMConfiguration::{Accessor,Attribute}Configuration::surrogatePropertyName,
I
> think we don't need this field.  When installing a property, we know its
> interface name and property name, so we can create a string on the fly.  I
think
> we only need a single bit to specify that it should be
> FunctionTemplate::NewSurrogate.
> 
> nit: I'd recommend "Surrogate#<interface_name>#<property_name>" rather than
> "<interface_name>#<property_name>", which may conflict with other use cases of
> v8::Private.

You are totally right, indeed, that was the original idea. But having access
"anywhere" to the private property greatly simplifies invalidation/update.
Consider the updateDocumentWrapper function:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

There's no clean way to update the hidden property, you'd need to scrap the
context->Global() object to access 'document' (which holds the v8::Private) to
finally update it.

Caching the private property makes invalidation/update a one-liner.

[haraken, 2016-09-16 08:46:59]

On 2016/09/16 08:46:20, Alfonso wrote:
> On 2016/09/16 08:09:31, Yuki wrote:
> > For WindowProxy::cachedAccessors(), IIUC, I don't see much benefit to cache
> > them.  Please correct me if I'm misunderstanding.
> > 
> > A v8::Private for a certain accessor property is created at
> > V8DOMConfiguration::createAccessorFunctionOrTemplate() and used (almost)
only
> > once at V8DOMConfiguration::installAccessorInternal().  Because v8::Private
is
> > per-isolate and v8::FunctionTemplate is also per-isolate, it's very rare to
> > reuse the v8::Private because 
> > we hardly re-install the same interface's same property again.  Seeing it's
> used
> > almost only once, is it worth caching them?  It seems to me we don't need
this
> > cache at all.
> > 
> > 
> > For
> >
V8DOMConfiguration::{Accessor,Attribute}Configuration::surrogatePropertyName,
> I
> > think we don't need this field.  When installing a property, we know its
> > interface name and property name, so we can create a string on the fly.  I
> think
> > we only need a single bit to specify that it should be
> > FunctionTemplate::NewSurrogate.
> > 
> > nit: I'd recommend "Surrogate#<interface_name>#<property_name>" rather than
> > "<interface_name>#<property_name>", which may conflict with other use cases
of
> > v8::Private.
> 
> You are totally right, indeed, that was the original idea. But having access
> "anywhere" to the private property greatly simplifies invalidation/update.
> Consider the updateDocumentWrapper function:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> 
> There's no clean way to update the hidden property, you'd need to scrap the
> context->Global() object to access 'document' (which holds the v8::Private) to
> finally update it.
> 
> Caching the private property makes invalidation/update a one-liner.

+1

[Yuki, 2016-09-16 11:53:54]

On 2016/09/16 08:46:59, haraken wrote:
> On 2016/09/16 08:46:20, Alfonso wrote:
> > On 2016/09/16 08:09:31, Yuki wrote:
> > > For WindowProxy::cachedAccessors(), IIUC, I don't see much benefit to
cache
> > > them.  Please correct me if I'm misunderstanding.
> > > 
> > > A v8::Private for a certain accessor property is created at
> > > V8DOMConfiguration::createAccessorFunctionOrTemplate() and used (almost)
> only
> > > once at V8DOMConfiguration::installAccessorInternal().  Because
v8::Private
> is
> > > per-isolate and v8::FunctionTemplate is also per-isolate, it's very rare
to
> > > reuse the v8::Private because 
> > > we hardly re-install the same interface's same property again.  Seeing
it's
> > used
> > > almost only once, is it worth caching them?  It seems to me we don't need
> this
> > > cache at all.
> > > 
> > > 
> > > For
> > >
> V8DOMConfiguration::{Accessor,Attribute}Configuration::surrogatePropertyName,
> > I
> > > think we don't need this field.  When installing a property, we know its
> > > interface name and property name, so we can create a string on the fly.  I
> > think
> > > we only need a single bit to specify that it should be
> > > FunctionTemplate::NewSurrogate.
> > > 
> > > nit: I'd recommend "Surrogate#<interface_name>#<property_name>" rather
than
> > > "<interface_name>#<property_name>", which may conflict with other use
cases
> of
> > > v8::Private.
> > 
> > You are totally right, indeed, that was the original idea. But having access
> > "anywhere" to the private property greatly simplifies invalidation/update.
> > Consider the updateDocumentWrapper function:
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > 
> > There's no clean way to update the hidden property, you'd need to scrap the
> > context->Global() object to access 'document' (which holds the v8::Private)
to
> > finally update it.
> > 
> > Caching the private property makes invalidation/update a one-liner.
> 
> +1

I see.  Then, I now think it's better to build V8PrivateProperty on top of your
string => v8::Private map, rather than using V8PrivateProperty in this CL.  The
current implementation of V8PrivateProperty does not support any caching for an
arbitrary string.

Now my only concern is use of std::string and std::map.  Blink has its own
WTFString/AtomicString/etc. and HashMap.

[Alfonso, 2016-09-16 13:22:23]

Description was changed from

==========
Add [StoreInField] attribute to cache (almost) constant accessors e.g. document.
Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);

BUG=chromium:634276
==========

to

==========
Add [StoreInField] attribute to cache (almost) constant accessors e.g. document.
Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);


To invalidate/update (e.g. window.document) just use:

V8PrivateProperty::getWindow_document().set(context, window,
newDocumentWrapper);

BUG=chromium:634276
==========

[Alfonso, 2016-09-16 14:21:48]

PTAL.
Refactored to use V8PrivateProperty, IMHO is way cleaner now.

I addressed all the comments, except those related to naming. Meaningful naming
is hard, the current naming is not definitive. The problem is that technically
it's not a cache (no checks on read), and it's not constant (can be updated).
Suggestions are very welcome.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:110:
v8::String::NewFromUtf8(isolate, surrogateName,
v8::NewStringType::kNormal).ToLocalChecked();
On 2016/09/15 15:29:47, vogelheim wrote:
> nitpick: The line breaking is strange. Since Blink doesn't have an 80 columns
> rule, maybe write it all in one line?

Done.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:136:
v8::Local<v8::FunctionTemplate> functionTemplate =
createAccessorFunctionOrTemplate<v8::FunctionTemplate>(isolate, callback,
nullptr, 0, data, signature, length);
On 2016/09/15 08:20:51, jochen wrote:
> nullptr

I'm confused about when to use 0 vs nullptr, I've read at least two
contradicting guidelines.
Done.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:187:
instanceOrTemplate->Set(v8::Private::AsSymbol(property).As<v8::Name>(),
v8::Undefined(isolate));
On 2016/09/15 08:20:51, jochen wrote:
> undefined would be the wrong value here, shouldn't that be whatever the getter
> returns?
> 
> Do layout tests pass with this?

I only have ran V8-side tests, but it will be removed anyway.
Done.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:187:
instanceOrTemplate->Set(v8::Private::AsSymbol(property).As<v8::Name>(),
v8::Undefined(isolate));
On 2016/09/15 08:20:51, jochen wrote:
> undefined would be the wrong value here, shouldn't that be whatever the getter
> returns?
> 
> Do layout tests pass with this?

Ack-ed.
haraken@ also suggested to use the normal callback to initialize it, but I'm not
sure if everything is correctly initialized at this point.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:416:
v8::Local<v8::Private> priv =
cachedAccessors().at("Window#document").Get(m_isolate);
On 2016/09/15 08:17:34, Yuki wrote:
> Can you use V8PrivateProperty?
> 
> I think std::map and std::string are discouraged in Blink.

Done.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:420:
CHECK(context->Global()->ForceSet(context, v8::Private::AsSymbol(priv),
documentWrapper).FromJust());
On 2016/09/15 08:20:51, jochen wrote:
> why forceset?

Done.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:93:
DCHECK(isMainThread());
On 2016/09/15 08:20:51, jochen wrote:
> that means that this feature is currently not available to workers, but since
> it's on window proxy that's fine I guess

Refactor to use V8PrivateProperty to expose private properties, this is no
longer needed/used.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:94:
DEFINE_STATIC_LOCAL(StringPrivateMap, map, ());
On 2016/09/15 09:26:10, haraken wrote:
> 
> If this map is intended to be per-isolate, can you put the map on
> V8PerIsolateData? WindowProxy doesn't look like a right place to put it.
> 
> That said, as yukishiino@ mentioned, if you can use V8PrivateProperty, that
will
> be the best solution.

Done.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (left):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:122:
'has_fast_accessor': 'FastAccessor' in extended_attributes,
On 2016/09/15 08:20:51, jochen wrote:
> unrelated?

The attribute is duplicated, will move it to the Fast-Accessors CL.
Ack-ed.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp:505: '0' %}
On 2016/09/15 08:20:51, jochen wrote:
> nullptr?

All the others callbacks seems to be using '0', I'm lost here.
Is this part of the updated guidelines, should I follow the new style or  mimic
the existing code?.

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/2335203006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp:700:
"{{method.name}}", {{getter_callback}}, {{setter_callback}},
{{getter_callback_for_main_world}}, {{setter_callback_for_main_world}}, nullptr,
0, &{{v8_class}}::wrapperTypeInfo, v8::ALL_CAN_READ, {{property_attribute}},
{{only_exposed_to_private_script}}, {{property_location(method)}},
{{holder_check}},
On 2016/09/15 08:20:51, jochen wrote:
> nullptr?

Done.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp:23: return
v8::Private::New(isolate, str);
::ForApi is backed by another internal cache, so further ::ForApi calls with the
same name will reuse the same symbol. But since it's only used to initialize the
symbol once, ::New does the same.

Additionally, ::ForApi needs a context which is not yet created when
initializing 'window'. A window object is initialized and then the context is
created with it as global object.

[Alfonso, 2016-09-19 13:00:36]

[StoreInField] => [Caching=Push]
I've been thinking about proper naming, here's the motivation:

Generally the plain term "cache" refers to a pull approach, with the advantage
is that recalculation occurs only on demand so the cache may remain dirty until
it's actually read/recalculated. The disadvantage is that the dirty-ness check
must be performed every single access.

In our case we are using a push approach, there's no overhead on read, but the
values must be always updated.

So I propose to name the IDL attribute and all it's associated parts (including
the V8-side) to reflect the fact that it's a push cache, it gives a straight
meaning to what it does and how it works (in both Blink and V8 sides).

The current [StoreInField] attribute would become [Caching=Push], and can be
later extended for other types such as Constant or Pull, which should be
straightforward to implement based on this CL, but just not needed at the
moment.

Please share your impressions/concerns.

[jochen (gone - plz use gerrit), 2016-09-19 14:54:19]

Cached=Push sgtm

Please also format your CL description according to
chromium.org/developers/contributing-code#Writing_change_list_descriptions

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:172:
v8::Local<FunctionOrTemplate> setter =
createAccessorFunctionOrTemplate<FunctionOrTemplate>(isolate, setterCallback,
nullptr, 0, data, signature, 1);
you still have some 0 in your CL that should be nullptr:
https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:417:
CHECK(V8PrivateProperty::getWindow_document(m_isolate).set(context,
context->Global(), documentWrapper));
I suspect that you need to do this also right after creating the context. I
would expect that fast/dom/Window/window-early-properties.html otherwise doesn't
pass

[haraken, 2016-09-20 05:53:25]

Implementation mostly looks good.

On 2016/09/19 13:00:36, Alfonso wrote:
> [StoreInField] => [Caching=Push]
> I've been thinking about proper naming, here's the motivation:
> 
> Generally the plain term "cache" refers to a pull approach, with the advantage
> is that recalculation occurs only on demand so the cache may remain dirty
until
> it's actually read/recalculated. The disadvantage is that the dirty-ness check
> must be performed every single access.
> 
> In our case we are using a push approach, there's no overhead on read, but the
> values must be always updated.
> 
> So I propose to name the IDL attribute and all it's associated parts
(including
> the V8-side) to reflect the fact that it's a push cache, it gives a straight
> meaning to what it does and how it works (in both Blink and V8 sides).
> 
> The current [StoreInField] attribute would become [Caching=Push], and can be
> later extended for other types such as Constant or Pull, which should be
> straightforward to implement based on this CL, but just not needed at the
> moment.
> 
> Please share your impressions/concerns.

I'd prefer using [AttributeCaching] without any =Pull|Push|Constant because:

- I cannot imagine any use case for =Pull.
- It won't be worth distinguishing =Push from =Constant. =Constant is just a
special version of =Pull. If the embedder doesn't update the cache, it's a
constant cache.

Either way, if you unify the name for "surrogate", "StoreInField" to something
better, LG.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1579: Summary:
Caches invariable accessors to a private property. Private property name is
automatically generated e.g. "Class#attribute".

Make it clear that this caches a return value of DOM attribute getters that
don't have any side effect. You shouldn't use this feature for DOM operations
that may have side effects.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:172:
v8::Local<FunctionOrTemplate> setter =
createAccessorFunctionOrTemplate<FunctionOrTemplate>(isolate, setterCallback,
nullptr, 0, data, signature, 1);
On 2016/09/19 14:54:19, jochen wrote:
> you still have some 0 in your CL that should be nullptr:
> https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL

You don't need to worry about 0 or nullptr in this CL. It's already randomly
mixed in the IDL compiler -- which should be a separate issue.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:69: #include <map>

Remove this.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:43: #include <map>

Remove this.

[Yuki, 2016-09-20 07:53:53]

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:38: X(Window,
_document)
nit: Why not X(Window, Document)?

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:65:
'bindings/core/v8/V8PrivateProperty.h',
nit: It's better to include the header iff you really need it.

[Alfonso, 2016-09-20 13:42:14]

Patchset #6 (id:100001) has been deleted

[Alfonso, 2016-09-20 13:46:24]

Description was changed from

==========
Add [StoreInField] attribute to cache (almost) constant accessors e.g. document.
Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);


To invalidate/update (e.g. window.document) just use:

V8PrivateProperty::getWindow_document().set(context, window,
newDocumentWrapper);

BUG=chromium:634276
==========

to

==========
Add [CachedAccessor] attribute to cache (almost) constant accessors.

Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);


To invalidate/update (e.g. window.document) just use:

V8PrivateProperty::getWindowDocument().set(context, window, newDocumentWrapper);

BUG=chromium:634276
==========

[Alfonso, 2016-09-20 13:48:24]

Naming revisited + some cleanup, PTAL.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1579: Summary:
Caches invariable accessors to a private property. Private property name is
automatically generated e.g. "Class#attribute".
On 2016/09/20 05:53:25, haraken wrote:
> 
> Make it clear that this caches a return value of DOM attribute getters that
> don't have any side effect. You shouldn't use this feature for DOM operations
> that may have side effects.
> 

Done.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:38: X(Window,
_document)
On 2016/09/20 07:53:53, Yuki wrote:
> nit: Why not X(Window, Document)?

Done.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:69: #include <map>
On 2016/09/20 05:53:25, haraken wrote:
> 
> Remove this.

Done.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:417:
CHECK(V8PrivateProperty::getWindow_document(m_isolate).set(context,
context->Global(), documentWrapper));
Done. The test pass as is.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:43: #include <map>
On 2016/09/20 05:53:25, haraken wrote:
> 
> Remove this.

Done.

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:65:
'bindings/core/v8/V8PrivateProperty.h',
Could you indicate me where to add the include properly?, it's only needed in
V8Window.cpp.

[Yuki, 2016-09-20 13:57:19]

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2335203006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:65:
'bindings/core/v8/V8PrivateProperty.h',
On 2016/09/20 13:48:23, Alfonso wrote:
> Could you indicate me where to add the include properly?, it's only needed in
> V8Window.cpp.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...

You can follow this way, I think.

[haraken, 2016-09-20 14:15:03]

LGTM once the Intent-to-implement is approved.

Please use the name "CachedAccessor" through the CL consistently.

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1580: 

Add a webidl example.

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1581: 

Some description is needed to describe what [CachedAccessor] is. You can move
some of the description from "note".

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:37: CachedAccessor

Alphabetical order.

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

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:98:
v8::Local<v8::FunctionTemplate>
createAccessorFunctionOrTemplate<v8::FunctionTemplate>(v8::Isolate* isolate,
v8::FunctionCallback callback, V8DOMConfiguration::FastAccessorBuilderCallback
fastCallback, V8DOMConfiguration::PrivatePropertyCallback cacheProperty,
v8::Local<v8::Value> data, v8::Local<v8::Signature> signature, int length)

cachedAccessorCallback ?

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:68: typedef
v8::Local<v8::Private> (*PrivatePropertyCallback)(v8::Isolate*);

PrivatePropertyCallback => CachedAccessorCallback ?

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:84:
PrivatePropertyCallback cachePropertyCallback; // The accessor's 'result' is
stored in a private property.

cachedAccessorCallback ?

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:111:
PrivatePropertyCallback cachePropertyCallback; // The accessor's 'result' is
stored in a private property.

cachedAccessorCallback ?

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:38: X(Window,
Document)

I'd prefer X(Window, DocumentCachedAccessor) to avoid conflict with other
private properties.

(In long term, we might want to auto-generate the private properties for cached
accessors with the IDL compiler.)

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

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:415: // Update cache.

Update cached accessor.

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:175:
'cache_property_name': '%s%s' % (interface.name, attribute.name.capitalize()),

cached_accessor_name

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp:219: {% macro
attribute_cache_property_callback(attribute) %}

cached_accessor_callback

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp:220: static
v8::Local<v8::Private> {{attribute.name}}CachePropertyCallback(v8::Isolate*
isolate)

CachedAccessorCallback

https://codereview.chromium.org/2335203006/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp:511: {% set
cache_property_callback =

cached_accessor_callback

[jochen (gone - plz use gerrit), 2016-09-22 08:22:06]

Window.idl lgtm

please add a link to the intent to ship thread in the CL description

[Alfonso, 2016-09-26 21:10:39]

Description was changed from

==========
Add [CachedAccessor] attribute to cache (almost) constant accessors.

Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);


To invalidate/update (e.g. window.document) just use:

V8PrivateProperty::getWindowDocument().set(context, window, newDocumentWrapper);

BUG=chromium:634276
==========

to

==========
Add [CachedAccessor] attribute to cache (almost) constant accessors.

Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);


To invalidate/update (e.g. window.document) just use:

V8PrivateProperty::getWindowDocument().set(context, window, newDocumentWrapper);

BUG=chromium:634276

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ
==========

[blink-reviews, 2016-09-26 21:11:51]

I've tested turning some Window 'special' attributes into plain old
accessors and there's no difference in behavior: the generated code is the
same, and the getter is called every single time. I'm wondering why they
are declared as 'special' data properties instead of plain accessors.
On the other hand THERE IS a difference in performance e.g. 15% for .window
and 2X for .performance in favor of the 'special' data properties, I have
no explanation for this.

All these attributes in both cases are loaded using LoadGlobalGeneric which
is very very slow.

All (Window) attributes are treated as data properties (violating the
spec?) but without the real performance gain.

With the current 'ForceSet' hack for document we have the following in
Chrome stable (very simple benchmark, stress 10^8 accesses),

   - document ~ 200ms (ForceSet-ed data property)
   - w.document ~ 765ms (const w = window) (data property again)
   - window ~ 17 seconds ('special' data property)
   - window.document ~ 18 seconds

In Firefox all benchmarks take ~ the same time: 200ms.

Data property access (eg. document) is 90X faster than the current
'special' data properties (eg. window).
Maybe I'm missing something on how the 'special' data properties work.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-26 21:11:54]

I've tested turning some Window 'special' attributes into plain old
accessors and there's no difference in behavior: the generated code is the
same, and the getter is called every single time. I'm wondering why they
are declared as 'special' data properties instead of plain accessors.
On the other hand THERE IS a difference in performance e.g. 15% for .window
and 2X for .performance in favor of the 'special' data properties, I have
no explanation for this.

All these attributes in both cases are loaded using LoadGlobalGeneric which
is very very slow.

All (Window) attributes are treated as data properties (violating the
spec?) but without the real performance gain.

With the current 'ForceSet' hack for document we have the following in
Chrome stable (very simple benchmark, stress 10^8 accesses),

   - document ~ 200ms (ForceSet-ed data property)
   - w.document ~ 765ms (const w = window) (data property again)
   - window ~ 17 seconds ('special' data property)
   - window.document ~ 18 seconds

In Firefox all benchmarks take ~ the same time: 200ms.

Data property access (eg. document) is 90X faster than the current
'special' data properties (eg. window).
Maybe I'm missing something on how the 'special' data properties work.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2016-09-27 04:51:12]

On 2016/09/26 21:11:54, chromium-reviews wrote:
> I've tested turning some Window 'special' attributes into plain old
> accessors and there's no difference in behavior: the generated code is the
> same, and the getter is called every single time. I'm wondering why they
> are declared as 'special' data properties instead of plain accessors.

That's only for a historical reason. We want to change all Window's attributes
to accessor-type properties.

yukishiino@ tried to change that in https://codereview.chromium.org/1380503002/,
but the CL was reverted due to performance regression. I hope [CachedAccessor]
will be helpful to mitigate the perf regression (probably using [CachedAccessor]
on window.document is enough to mitigate the regression), and we can reland the
CL soon.

> On the other hand THERE IS a difference in performance e.g. 15% for .window
> and 2X for .performance in favor of the 'special' data properties, I have
> no explanation for this.
> 
> All these attributes in both cases are loaded using LoadGlobalGeneric which
> is very very slow.
> 
> All (Window) attributes are treated as data properties (violating the
> spec?) but without the real performance gain.
> 
> With the current 'ForceSet' hack for document we have the following in
> Chrome stable (very simple benchmark, stress 10^8 accesses),
> 
>    - document ~ 200ms (ForceSet-ed data property)
>    - w.document ~ 765ms (const w = window) (data property again)
>    - window ~ 17 seconds ('special' data property)
>    - window.document ~ 18 seconds
> 
> In Firefox all benchmarks take ~ the same time: 200ms.
> 
> Data property access (eg. document) is 90X faster than the current
> 'special' data properties (eg. window).
> Maybe I'm missing something on how the 'special' data properties work.

[jochen (gone - plz use gerrit), 2016-09-27 06:51:21]

assuming that the performance numbers after your patch are about the same, I'd
say ship it.

Window is already bad now, so it's ok if it stays bad - we can fix it later

[haraken, 2016-09-27 07:25:27]

On 2016/09/27 06:51:21, jochen (slow) wrote:
> assuming that the performance numbers after your patch are about the same, I'd
> say ship it.
> 
> Window is already bad now, so it's ok if it stays bad - we can fix it later

LGTM

[Alfonso, 2016-09-27 13:29:45]

I found two cases where the cached value is being completely ignored, and the
getter is called instead.

This happens when accessing document from window, namely 'window.document' and
'w.document' (for some variable w = window).
This is a very special case due to the dual nature of the window/GlobalProxy
object.

Right now those cases are 2 times slower (since the cached value isn't used at
all).

I'm working on speeding up window.* attributes please refer to
https://codereview.chromium.org/2369933005/ for further details.
This will benefit ALL window's attributes; making accesses to document and other
attributes even faster in some cases.

[Alfonso, 2016-09-27 13:31:08]

Description was changed from

==========
Add [CachedAccessor] attribute to cache (almost) constant accessors.

Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);


To invalidate/update (e.g. window.document) just use:

V8PrivateProperty::getWindowDocument().set(context, window, newDocumentWrapper);

BUG=chromium:634276

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ
==========

to

==========
Add [CachedAccessor] attribute to cache (almost) constant accessors.

Calls to getter are translated into a cheap property load, eliminating the
calling overhead. Other potential benefits could also apply, like inlining
and/or constant propagation.

Currently the 'document' DOM accessor is implemented as a plain data property
for performance reasons, with the downside of violating the spec. This produces
exactly the same (performant) code as the current hack while being
spec-compliant.

The following JS code is now valid:

var acc = Object.getOwnPropertyDescriptor(window, "document");
var d = acc.get.call(window);


To invalidate/update (e.g. window.document) just use:

V8PrivateProperty::getWindowDocument().set(context, window, newDocumentWrapper);

BUG=chromium:634276

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ


Specialize GlobalProxy access:
https://codereview.chromium.org/2369933005
==========

[haraken, 2016-10-04 09:38:34]

Is it likely that someone can land this CL?

[jochen (gone - plz use gerrit), 2016-10-04 09:40:44]

On 2016/10/04 at 09:38:34, haraken wrote:
> Is it likely that someone can land this CL?

yeah, we'll take care of that

[vogelheim, 2016-11-08 09:33:21]

This work landed as crrev.com/2429343004. Closing this CL.

[vogelheim, 2016-11-08 09:33:21]

This work landed as crrev.com/2429343004. Closing this CL.