[Bindings] Use SetNativeDataProperty to install features on instances

Use SetNativeDataProperty to install features dynamically on instances

This CL replace DefineOwnProperty with SetNativeDataProperty in
InstallAttributeInternal<v8::Object>() not to create function
templates or instances on feature installations.

This change enables us to use the function for all data properties
and it improves performance of preparing origin-trial features
and dynamic installs of runtime enabled features.


BUG=617892
Review-Url: https://codereview.chromium.org/2815453005
Cr-Commit-Position: refs/heads/master@{#467260}
Committed: https://chromium.googlesource.com/chromium/src/+/040755ece35f949c4de74d85f5a651e84a0beafe

[peria, 2017-04-12 06:51:31]

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

[commit-bot: I haz the power, 2017-04-12 06:51:46]

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

[peria, 2017-04-12 08:08:37]

Description was changed from

==========
Use SetAccessor to install feature interface on instances

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

to

==========
Use SetAccessor to install features dynamically on instances

This CL prevents InstallAttributeInternal to create function
templates and instances of all attributes.
It means that the installation finishes quickly, and each
feature will be instantiated lazily. So it improves performance.

If we enable snapshotting, JFYI, it improves the performance
to install runtime enabled features, because Window interface
installs hundreds of unused features in its instantiation.


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

[commit-bot: I haz the power, 2017-04-12 08:40:24]

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

[commit-bot: I haz the power, 2017-04-12 08:40:24]

Dry run: This issue passed the CQ dry run.

[peria, 2017-04-13 06:15:54]

Description was changed from

==========
Use SetAccessor to install features dynamically on instances

This CL prevents InstallAttributeInternal to create function
templates and instances of all attributes.
It means that the installation finishes quickly, and each
feature will be instantiated lazily. So it improves performance.

If we enable snapshotting, JFYI, it improves the performance
to install runtime enabled features, because Window interface
installs hundreds of unused features in its instantiation.


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

to

==========
Use SetAccessor to install features dynamically on instances

This CL prevents InstallAttributeInternal to create function
templates and instances.
It means that the installation finishes quickly, and each
feature will be instantiated lazily.

As a result, it improves the performance to install runtime
enabled features to deserialize a snapshotted context,
because Window interface installs hundreds of unused features
in its instantiation.


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

[peria, 2017-04-13 06:48:28]

Description was changed from

==========
Use SetAccessor to install features dynamically on instances

This CL prevents InstallAttributeInternal to create function
templates and instances.
It means that the installation finishes quickly, and each
feature will be instantiated lazily.

As a result, it improves the performance to install runtime
enabled features to deserialize a snapshotted context,
because Window interface installs hundreds of unused features
in its instantiation.


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

to

==========
Use SetAccessor to install features dynamically on instances

This CL prevents InstallAttributeInternal to create function
templates and instances.
It means that the installation finishes quickly, and each
feature will be instantiated lazily.

As a result, it improves the performance to install runtime
enabled features to deserialize a snapshotted context,
because Window interface installs hundreds of unused features
in its instantiation.


BUG=617892
==========

[peria, 2017-04-13 06:53:27]

Description was changed from

==========
Use SetAccessor to install features dynamically on instances

This CL prevents InstallAttributeInternal to create function
templates and instances.
It means that the installation finishes quickly, and each
feature will be instantiated lazily.

As a result, it improves the performance to install runtime
enabled features to deserialize a snapshotted context,
because Window interface installs hundreds of unused features
in its instantiation.


BUG=617892
==========

to

==========
Use SetAccessor to install features dynamically on instances

This CL prevents InstallAttributeInternal to create function
templates and instances.
It means that the installation finishes quickly, and Chrome
instantiates each feature at call time.


BUG=617892
==========

[peria, 2017-04-13 07:48:15]

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

[peria, 2017-04-13 07:48:33]

peria@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, yukishiino@chromium.org

[peria, 2017-04-13 07:48:34]

PTL

[commit-bot: I haz the power, 2017-04-13 07:48:47]

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

[haraken, 2017-04-13 08:06:09]

LGTM but let's wait for an approval from shiino-san.

https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (left):

https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:105:
DCHECK_EQ(&V8ConstructorAttributeGetter, attribute.getter);

Can we keep this assert?

This assert is important since non-data properties should not use
InstallAttributeInternal (which should actually be renamed to
InstallConstructorDataProperty).

Not related to this CL, but we should really consider renaming methods in this
file. They are confusing...

[bashi, 2017-04-13 08:08:36]

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

https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:109:
DCHECK(instance
Why do we need DCHECK()? ToChecked() should crash on failures.

[peria, 2017-04-13 09:51:21]

https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (left):

https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:105:
DCHECK_EQ(&V8ConstructorAttributeGetter, attribute.getter);
On 2017/04/13 08:06:09, haraken wrote:
> 
> Can we keep this assert?
> 
> This assert is important since non-data properties should not use
> InstallAttributeInternal (which should actually be renamed to
> InstallConstructorDataProperty).
> 

done

> Not related to this CL, but we should really consider renaming methods in this
> file. They are confusing...

Agreed. I'll rename these functions (and also local variables) in another CL.

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

https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:109:
DCHECK(instance
On 2017/04/13 08:08:36, bashi wrote:
> Why do we need DCHECK()? ToChecked() should crash on failures.

Hmm, I thought returning false can be independent from internal exceptions, but
as far as I read V8 code, SetAccessor seems to return iff it has a pending
exception.
So removed.

[peria, 2017-04-13 09:51:27]

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

[commit-bot: I haz the power, 2017-04-13 09:51:48]

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-13 11:35:16]

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

[commit-bot: I haz the power, 2017-04-13 11:35:17]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-04-13 14:54:45]

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

[jbroman, 2017-04-13 14:54:45]

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

https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:117:
->SetAccessor(context, name, getter, setter, data, v8::DEFAULT,
This is only for origin trials, so maybe you've already accepted it as okay, but
this does change what was previously a data property to now be an accessor
property, which is a visible change to authors.

(We don't currently expose an equivalent of SetNativeDataProperty or
SetLazyDataProperty on v8::Object like we do on v8::Template, but I think we
could.)

[haraken, 2017-04-13 15:40:19]

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

https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:117:
->SetAccessor(context, name, getter, setter, data, v8::DEFAULT,
On 2017/04/13 14:54:45, jbroman wrote:
> This is only for origin trials, so maybe you've already accepted it as okay,
but
> this does change what was previously a data property to now be an accessor
> property, which is a visible change to authors.
> 
> (We don't currently expose an equivalent of SetNativeDataProperty or
> SetLazyDataProperty on v8::Object like we do on v8::Template, but I think we
> could.)

The name is really confusing but:

- SetAccessor => data property
- SetAccessorProerpty => accessor property

if I'm understanding correctly.

[jbroman, 2017-04-13 17:11:24]

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

https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:117:
->SetAccessor(context, name, getter, setter, data, v8::DEFAULT,
On 2017/04/13 at 15:40:19, haraken wrote:
> On 2017/04/13 14:54:45, jbroman wrote:
> > This is only for origin trials, so maybe you've already accepted it as okay,
but
> > this does change what was previously a data property to now be an accessor
> > property, which is a visible change to authors.
> > 
> > (We don't currently expose an equivalent of SetNativeDataProperty or
> > SetLazyDataProperty on v8::Object like we do on v8::Template, but I think we
> > could.)
> 
> The name is really confusing but:
> 
> - SetAccessor => data property
> - SetAccessorProerpty => accessor property
> 
> if I'm understanding correctly.

Ah. If that's the case, then ignore me (though it is indeed confusing).

[Yuki, 2017-04-14 07:39:20]

The CL itself LGTM.

Talked with peria-san offline about another issue.  Let's fix it first.

[peria, 2017-04-20 03:56:19]

Description was changed from

==========
Use SetAccessor to install features dynamically on instances

This CL prevents InstallAttributeInternal to create function
templates and instances.
It means that the installation finishes quickly, and Chrome
instantiates each feature at call time.


BUG=617892
==========

to

==========
Use SetNativeDataProperty to install features dynamically on instances

This CL replace DefineOwnProperty with SetNativeDataProperty in
InstallAttributeInternal<v8::Object>() not to create function
templates or instances on feature installations.

This change enables us to use the function for all data properties
and it improves performance of preparing origin-trial features
and dynamic installs of runtime enabled features.


BUG=617892
==========

[peria, 2017-04-20 04:01:31]

I prepared v8::Object::SetNativeDataProperty()[1], and update
this CL to use it.
Please take another look.


[1]
https://chromium.googlesource.com/v8/v8.git/+/1da951ad0bd1a3c2247863060b87ade...

[peria, 2017-04-20 04:01:37]

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

[commit-bot: I haz the power, 2017-04-20 04:02:42]

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-20 05:47:06]

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

[commit-bot: I haz the power, 2017-04-20 05:47:09]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-04-20 09:12:38]

LGTM.

[peria, 2017-04-26 05:35:37]

The CQ bit was checked by peria@chromium.org

[peria, 2017-04-26 05:35:38]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2815453005/#ps80001
(title: "Use SetNativeDataProperty")

[commit-bot: I haz the power, 2017-04-26 05:35:56]

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-26 07:20:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493184937360250,
"parent_rev": "9e289b3dac38afcb47438b000622854a0560ff43", "commit_rev":
"040755ece35f949c4de74d85f5a651e84a0beafe"}

[commit-bot: I haz the power, 2017-04-26 07:21:04]

Description was changed from

==========
Use SetNativeDataProperty to install features dynamically on instances

This CL replace DefineOwnProperty with SetNativeDataProperty in
InstallAttributeInternal<v8::Object>() not to create function
templates or instances on feature installations.

This change enables us to use the function for all data properties
and it improves performance of preparing origin-trial features
and dynamic installs of runtime enabled features.


BUG=617892
==========

to

==========
Use SetNativeDataProperty to install features dynamically on instances

This CL replace DefineOwnProperty with SetNativeDataProperty in
InstallAttributeInternal<v8::Object>() not to create function
templates or instances on feature installations.

This change enables us to use the function for all data properties
and it improves performance of preparing origin-trial features
and dynamic installs of runtime enabled features.


BUG=617892

Review-Url: https://codereview.chromium.org/2815453005
Cr-Commit-Position: refs/heads/master@{#467260}
Committed:
https://chromium.googlesource.com/chromium/src/+/040755ece35f949c4de74d85f5a6...
==========

[commit-bot: I haz the power, 2017-04-26 07:21:05]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/040755ece35f949c4de74d85f5a6...