V8 support for cached accessors.

V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

BUG=chromium:634276, v8:5548
Committed: https://crrev.com/cadcd787cf185f9fb974a1f0c12fdd13c45171e9
Cr-Commit-Position: refs/heads/master@{#40765}

[vogelheim, 2016-10-18 09:17:11]

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

[commit-bot: I haz the power, 2016-10-18 09:17:14]

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, 2016-10-18 09:44:57]

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

[commit-bot: I haz the power, 2016-10-18 09:44:58]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2016-10-18 09:58:39]

vogelheim@chromium.org changed reviewers:
+ jochen@chromium.org, verwaest@chromium.org

[vogelheim, 2016-10-18 09:58:39]

Please take a look.

This is based on crrev.com/2347523003, rebased. Also, based on Jochen's previous
feedback I moved the lookup logic into LookupIterator and use it from
{hydrogen,objects,ic}.cc

[jochen (gone - plz use gerrit), 2016-10-19 14:31:08]

lgtm

[jochen (gone - plz use gerrit), 2016-10-19 14:31:25]

can you please file a bug that this needs to be ported to TF?

[vogelheim, 2016-10-19 16:09:59]

On 2016/10/19 14:31:25, jochen wrote:
> can you please file a bug that this needs to be ported to TF?

Done: https://crbug.com/v8/5548

[vogelheim, 2016-10-19 16:10:29]

Description was changed from

==========
V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

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

to

==========
V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

BUG=chromium:634276, v8:5548
==========

[vogelheim, 2016-10-19 16:10:29]

Description was changed from

==========
V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

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

to

==========
V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

BUG=chromium:634276, v8:5548
==========

[vogelheim, 2016-10-20 14:07:14]

On 2016/10/19 16:09:59, vogelheim wrote:
> On 2016/10/19 14:31:25, jochen wrote:
> > can you please file a bug that this needs to be ported to TF?
> 
> Done: https://crbug.com/v8/5548

Toon, can you do owner's review for src/{crankshaft,ic}/* ?

[Toon Verwaest, 2016-10-21 08:22:14]

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (left):

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc#oldcode1335
src/ic/ic.cc:1335: DCHECK(!receiver_is_holder);
Why is this removed? I'm pretty sure we need this. If this DCHECK fails, it's
probably not correct.

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc#newcode1282
src/ic/ic.cc:1282: if (lookup->TryLookupCacheProperty()) {
Shouldn't this be moved in the IsAccessorPair case?

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc#newcode1284
src/ic/ic.cc:1284: return CompileHandler(lookup, unused, cache_holder);
Don't we have to go through ComputeHandler again?

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.cc#newcode842
src/lookup.cc:842: if (!accessors->IsAccessorPair()) return false;
You should be able to make this a DCHECK and only call here if we are in t he
ACCESSOR + AccessorPair case.

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.cc#newcode845
src/lookup.cc:845: isolate());
you don't need isolate() here

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.h
File src/lookup.h (right):

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.h#newcode267
src/lookup.h:267: bool TryLookupCacheProperty();
TryLookupCache*d*Property

https://codereview.chromium.org/2405213002/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2405213002/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:5866: ACCESSORS(FunctionTemplateInfo, cache_property, Object,
kCachePropertyOffset)
cached_property or property_cache? What about cache_name?

https://codereview.chromium.org/2405213002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2405213002/diff/20001/src/objects.cc#newcode1330
src/objects.cc:1330: return Object::GetProperty(it);
Why not do GetProperty directly in TryLookupCachedProperty?

https://codereview.chromium.org/2405213002/diff/20001/src/objects.cc#newcode2...
src/objects.cc:20228: if (getter->IsFunctionTemplateInfo()) {
It should be easy to support JSFunctions backed by FunctionTemplateInfo as well.

[vogelheim, 2016-11-02 12:53:37]

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

[commit-bot: I haz the power, 2016-11-02 12:53: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, 2016-11-02 13:20:45]

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

[commit-bot: I haz the power, 2016-11-02 13:20:46]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2016-11-03 15:44:28]

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

[commit-bot: I haz the power, 2016-11-03 15:44:36]

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, 2016-11-03 16:11:34]

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

[commit-bot: I haz the power, 2016-11-03 16:11:35]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2016-11-03 16:12:24]

ptal

Didn't do two of the suggestions. Let me know if I'm overlooking something.

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (left):

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc#oldcode1335
src/ic/ic.cc:1335: DCHECK(!receiver_is_holder);
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> Why is this removed? I'm pretty sure we need this. If this DCHECK fails, it's
> probably not correct.

Done.

(I guess this was an artefact of the Comp[ile|ute]Handler thing above, and was
resolved by calling ComputeHandler.)

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc#newcode1282
src/ic/ic.cc:1282: if (lookup->TryLookupCacheProperty()) {
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> Shouldn't this be moved in the IsAccessorPair case?

Done.

https://codereview.chromium.org/2405213002/diff/20001/src/ic/ic.cc#newcode1284
src/ic/ic.cc:1284: return CompileHandler(lookup, unused, cache_holder);
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> Don't we have to go through ComputeHandler again?

Done.

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.cc#newcode842
src/lookup.cc:842: if (!accessors->IsAccessorPair()) return false;
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> You should be able to make this a DCHECK and only call here if we are in t he
> ACCESSOR + AccessorPair case.

Mostly done.

There's now a LookupCachedProperty, that assumes the caller has already
determined the iterator points to an accessor pair. That's awkward to use from
the hydrogen callsite, to I've also made a TryLookupCachedProperty that still
does the checks and calls LookupCachedProperty is the checks are successful.

I could also inline the Try* version, if that's considered more readable.

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.cc#newcode845
src/lookup.cc:845: isolate());
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> you don't need isolate() here

The compiler disagrees. With only one param, this calls a constructor
Handle(object, object->GetIsolate()), and Handle<Object> doesn't seem to have
->GetIsolate().

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.h
File src/lookup.h (right):

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.h#newcode267
src/lookup.h:267: bool TryLookupCacheProperty();
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> TryLookupCache*d*Property

Done.

https://codereview.chromium.org/2405213002/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2405213002/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:5866: ACCESSORS(FunctionTemplateInfo, cache_property, Object,
kCachePropertyOffset)
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> cached_property or property_cache? What about cache_name?

I picked cached_property_name. :)

Every other function related to this property is (now) called.
Something*CachedProperty*Something. The consistent naming should make this
easier to understand (and better searchable). Bit since this actually is the
name, I added used a 'name' suffix on this one.

https://codereview.chromium.org/2405213002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2405213002/diff/20001/src/objects.cc#newcode1330
src/objects.cc:1330: return Object::GetProperty(it);
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> Why not do GetProperty directly in TryLookupCachedProperty?

Hmm. That would seem to do the wrong thing for the other callsites in ic.cc or
hydrogen.cc. Am I overlooking something?

https://codereview.chromium.org/2405213002/diff/20001/src/objects.cc#newcode2...
src/objects.cc:20228: if (getter->IsFunctionTemplateInfo()) {
On 2016/10/21 08:22:14, Toon Verwaest wrote:
> It should be easy to support JSFunctions backed by FunctionTemplateInfo as
well.

Do we have a use case for that?

(I'm generally fond of the mechanism we create here, but right now there's only
a single 'real' use case, the document accessor. I'd prefer to not add another
case until we actually have use for it.)

[vogelheim, 2016-11-03 16:13:52]

vogelheim@chromium.org changed reviewers:
+ jkummerow@chromium.org

[vogelheim, 2016-11-03 16:13:53]

@jkummerow: Care to have a look at the changes ic.*? As briefly discussed in
person, I tried to make it pretty; I'm not sure if successful. :)

[Toon Verwaest, 2016-11-04 10:19:52]

lgtm

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/2405213002/diff/20001/src/lookup.cc#newcode845
src/lookup.cc:845: isolate());
On 2016/11/03 16:12:23, vogelheim wrote:
> On 2016/10/21 08:22:14, Toon Verwaest wrote:
> > you don't need isolate() here
> 
> The compiler disagrees. With only one param, this calls a constructor
> Handle(object, object->GetIsolate()), and Handle<Object> doesn't seem to have
> ->GetIsolate().

You're right; I misread I guess. Object includes Smi which doesn't live on the
heap so we can't get to the isolate.

https://codereview.chromium.org/2405213002/diff/60001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2405213002/diff/60001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:6162: if (cache_info.CanAccessMonomorphic())
Use {} if multi-line

https://codereview.chromium.org/2405213002/diff/60001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/2405213002/diff/60001/src/lookup.cc#newcode854
src/lookup.cc:854: Handle<Object>
getter(Handle<AccessorPair>::cast(GetAccessors())->getter(),
AccessorPair::cast(*GetAccessors())->getter(), isolate()) might fit on one line
:)

[vogelheim, 2016-11-04 12:34:32]

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

[commit-bot: I haz the power, 2016-11-04 12:34:34]

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

[vogelheim, 2016-11-04 12:34:42]

https://codereview.chromium.org/2405213002/diff/60001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2405213002/diff/60001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:6162: if (cache_info.CanAccessMonomorphic())
On 2016/11/04 10:19:52, Toon Verwaest wrote:
> Use {} if multi-line

Done.

https://codereview.chromium.org/2405213002/diff/60001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/2405213002/diff/60001/src/lookup.cc#newcode854
src/lookup.cc:854: Handle<Object>
getter(Handle<AccessorPair>::cast(GetAccessors())->getter(),
On 2016/11/04 10:19:52, Toon Verwaest wrote:
> AccessorPair::cast(*GetAccessors())->getter(), isolate()) might fit on one
line
> :)

Didn't fit :)
Since I need two lines anyway, I gave the pointer a name to make a tiny bit
prettier.

[Jakob Kummerow, 2016-11-04 12:39:18]

lgtm

[commit-bot: I haz the power, 2016-11-04 13:00:04]

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

[commit-bot: I haz the power, 2016-11-04 13:00:05]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2016-11-04 13:00:39]

The CQ bit was checked by vogelheim@chromium.org

[vogelheim, 2016-11-04 13:00:40]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2405213002/#ps80001
(title: "Toon's feedback.")

[commit-bot: I haz the power, 2016-11-04 13:00:45]

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, 2016-11-04 13:02:44]

Description was changed from

==========
V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

BUG=chromium:634276, v8:5548
==========

to

==========
V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

BUG=chromium:634276, v8:5548
==========

[commit-bot: I haz the power, 2016-11-04 13:02:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-17 22:22:47]

Description was changed from

==========
V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

BUG=chromium:634276, v8:5548
==========

to

==========
V8 support for cached accessors.

Some accessors requires little to no computation at all, its result can be
cached in a private property, avoiding the call overhead.
Calls to the getter are translated into a cheap property load.

Follow-on to crrev.com/2347523003, from peterssen@google.com

BUG=chromium:634276, v8:5548

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

[commit-bot: I haz the power, 2016-11-17 22:22:48]

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