Do not eagerly instantiate accessors' JSFunction.

Do not eagerly instantiate accessors' JSFunction.

BUG=

Committed: https://crrev.com/4d46b510caf534d770ce19a01a11b8796304471b
Cr-Commit-Position: refs/heads/master@{#33812}

Committed: https://crrev.com/e3458155994424801d7f22cd8bc21347266b2c4d
Cr-Commit-Position: refs/heads/master@{#33851}

[epertoso, 2016-01-20 10:59:51]

Not really ready for review yet. I kind of dislike the unchecked cast going on
in BuiltingArguments::target, but I'd like to know if this is the right
direction.

I run the blink_perf.bindings benchmarks and a few dromeo ones, the differences
between this and ToT aren't significant.

[Toon Verwaest, 2016-01-22 12:11:36]

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

[Toon Verwaest, 2016-01-22 12:11:36]

Agree that cast looks pretty ugly. Otoh I don't think it should block the entire
thing... The overall approach looks good.

https://codereview.chromium.org/1609233002/diff/40001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1609233002/diff/40001/src/builtins.cc#newcode...
src/builtins.cc:3417: :
Handle<FunctionTemplateInfo>(Handle<JSFunction>::cast(function)
Shorter: handle(JSFunction::cast(*function)->shared()->get_api_func_data());

[epertoso, 2016-01-22 14:36:35]

https://codereview.chromium.org/1609233002/diff/40001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1609233002/diff/40001/src/builtins.cc#newcode...
src/builtins.cc:3417: :
Handle<FunctionTemplateInfo>(Handle<JSFunction>::cast(function)
On 2016/01/22 at 12:11:36, Toon Verwaest wrote:
> Shorter: handle(JSFunction::cast(*function)->shared()->get_api_func_data());

Done.

[epertoso, 2016-01-22 14:36:48]

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

[commit-bot: I haz the power, 2016-01-22 14:36:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1609233002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1609233002/60001

[commit-bot: I haz the power, 2016-01-22 15:04:33]

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

[commit-bot: I haz the power, 2016-01-22 15:04:33]

Dry run: This issue passed the CQ dry run.

[epertoso, 2016-01-27 16:30:25]

Patchset #5 (id:80001) has been deleted

[epertoso, 2016-01-27 16:31:01]

I specialized BuiltinArguments::target. Please take another look.

[Toon Verwaest, 2016-01-28 12:05:19]

Some additional remarks.

https://codereview.chromium.org/1609233002/diff/100001/src/api-natives.cc
File src/api-natives.cc (right):

https://codereview.chromium.org/1609233002/diff/100001/src/api-natives.cc#new...
src/api-natives.cc:45: PropertyAttributes attributes) {
To default to not instantiating the FunctionTemplates, we definitely need to
DCHECK here that the templates are marked as caching:

DCHECK(!accessor->do_not_cache());

Note that this changes V8's API.

https://codereview.chromium.org/1609233002/diff/100001/src/crankshaft/hydroge...
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1609233002/diff/100001/src/crankshaft/hydroge...
src/crankshaft/hydrogen.cc:6584:
info->NeedsWrappingFor(Handle<JSFunction>::cast(info->accessor()))) {
If you could get a FunctionTemplate here, shouldn't you be able to get a
function template below as well where you added the DCHECK (in case inlining
fails for whatever reason)?

https://codereview.chromium.org/1609233002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1609233002/diff/100001/src/objects.cc#newcode...
src/objects.cc:10941: set(component, *function.ToHandleChecked());
If we'd like to share the accessorpair with FunctionTemplates between maps from
different origins, we can't mutate the pair in place. How hard would it be to
already put this copying in place?

[epertoso, 2016-02-01 16:18:04]

https://codereview.chromium.org/1609233002/diff/100001/src/api-natives.cc
File src/api-natives.cc (right):

https://codereview.chromium.org/1609233002/diff/100001/src/api-natives.cc#new...
src/api-natives.cc:45: PropertyAttributes attributes) {
On 2016/01/28 at 12:05:19, Toon Verwaest wrote:
> To default to not instantiating the FunctionTemplates, we definitely need to
DCHECK here that the templates are marked as caching:
> 
> DCHECK(!accessor->do_not_cache());
> 
> Note that this changes V8's API.

Done. But I don't get how this changes the API. We only set do_not_cache when we
create a new v8::Function (and that's fine, because we immediately instantiate
the template).

https://codereview.chromium.org/1609233002/diff/100001/src/crankshaft/hydroge...
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1609233002/diff/100001/src/crankshaft/hydroge...
src/crankshaft/hydrogen.cc:6584:
info->NeedsWrappingFor(Handle<JSFunction>::cast(info->accessor()))) {
On 2016/01/28 at 12:05:19, Toon Verwaest wrote:
> If you could get a FunctionTemplate here, shouldn't you be able to get a
function template below as well where you added the DCHECK (in case inlining
fails for whatever reason)?

Technically, it seems unlikely that TryInlineGetter/Setter would fail with an
API call.

Is it better to bailout if we reach that point with a FunctionTemplateInfo (see
below)?

https://codereview.chromium.org/1609233002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1609233002/diff/100001/src/objects.cc#newcode...
src/objects.cc:10941: set(component, *function.ToHandleChecked());
On 2016/01/28 at 12:05:19, Toon Verwaest wrote:
> If we'd like to share the accessorpair with FunctionTemplates between maps
from different origins, we can't mutate the pair in place. How hard would it be
to already put this copying in place?

I modified JSReceiver::GetOwnPropertyDescriptor and JSObject::GetAccessor.

[haraken, 2016-02-05 11:05:12]

(I'm just curious but would the LazyAccessorpair be available in the near
future? I'm not intending to rush you; just want to know the current status :-)

[Toon Verwaest, 2016-02-08 10:07:43]

lgtm with a final comment

https://codereview.chromium.org/1609233002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1609233002/diff/140001/src/objects.cc#newcode...
src/objects.cc:7281: it->TransitionToAccessorPair(new_pair,
it->property_attributes());
You can actually not install the instantiated accessors here. Otherwise
instantiating them from e.g., Realm A would install accessors from Realm A.
Later reading the accessors from Realm B would reveal that Realm A read them by
returning Relam A accessors. Just don't install it, and create a new object on
each read. This is a slow path anyway.

[epertoso, 2016-02-08 10:53:29]

https://codereview.chromium.org/1609233002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1609233002/diff/140001/src/objects.cc#newcode...
src/objects.cc:7281: it->TransitionToAccessorPair(new_pair,
it->property_attributes());
On 2016/02/08 at 10:07:43, Toon Verwaest wrote:
> You can actually not install the instantiated accessors here. Otherwise
instantiating them from e.g., Realm A would install accessors from Realm A.
Later reading the accessors from Realm B would reveal that Realm A read them by
returning Relam A accessors. Just don't install it, and create a new object on
each read. This is a slow path anyway.

Done.

[epertoso, 2016-02-08 11:19:42]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-02-08 11:19:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1609233002/#ps160001
(title: "Update")

[commit-bot: I haz the power, 2016-02-08 11:19:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1609233002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1609233002/160001

[commit-bot: I haz the power, 2016-02-08 11:28:46]

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

[commit-bot: I haz the power, 2016-02-08 11:28:47]

Try jobs failed on following builders:
  v8_win64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/2599)

[epertoso, 2016-02-08 11:57:34]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-02-08 11:57:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1609233002/#ps180001
(title: "Update.")

[commit-bot: I haz the power, 2016-02-08 11:57:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1609233002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1609233002/180001

[commit-bot: I haz the power, 2016-02-08 12:25:39]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-02-08 12:26:23]

Description was changed from

==========
Do not eagerly instantiate accessors' JSFunction.

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

to

==========
Do not eagerly instantiate accessors' JSFunction.

BUG=

Committed: https://crrev.com/4d46b510caf534d770ce19a01a11b8796304471b
Cr-Commit-Position: refs/heads/master@{#33812}
==========

[commit-bot: I haz the power, 2016-02-08 12:26:24]

Patchset 9 (id:??) landed as
https://crrev.com/4d46b510caf534d770ce19a01a11b8796304471b
Cr-Commit-Position: refs/heads/master@{#33812}

[Michael Achenbach, 2016-02-08 12:47:55]

A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/1679683004/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks gcmole:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gcmole/build....

[epertoso, 2016-02-08 14:37:55]

I fixed the issue GCMole was complaining about (in runtime-debug.cc).
AccessorPair::GetComponent is now static. 

Please, take another look.

[Toon Verwaest, 2016-02-09 15:55:44]

lgtm

[epertoso, 2016-02-09 15:57:30]

The CQ bit was checked by epertoso@chromium.org

[commit-bot: I haz the power, 2016-02-09 15:57:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1609233002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1609233002/200001

[commit-bot: I haz the power, 2016-02-09 16:28:20]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-02-09 16:28:55]

Description was changed from

==========
Do not eagerly instantiate accessors' JSFunction.

BUG=

Committed: https://crrev.com/4d46b510caf534d770ce19a01a11b8796304471b
Cr-Commit-Position: refs/heads/master@{#33812}
==========

to

==========
Do not eagerly instantiate accessors' JSFunction.

BUG=

Committed: https://crrev.com/4d46b510caf534d770ce19a01a11b8796304471b
Cr-Commit-Position: refs/heads/master@{#33812}

Committed: https://crrev.com/e3458155994424801d7f22cd8bc21347266b2c4d
Cr-Commit-Position: refs/heads/master@{#33851}
==========

[commit-bot: I haz the power, 2016-02-09 16:28:56]

Patchset 10 (id:??) landed as
https://crrev.com/e3458155994424801d7f22cd8bc21347266b2c4d
Cr-Commit-Position: refs/heads/master@{#33851}