[accessors] second-chance typed array field lookup

TypedArray accessor detection: consider entire prototype chain

When looking up a special accessor for known TypedArray fields
("length", "byteLength", "byteOffset"), consider the entire prototype
chain, not only the direct prototype.
This allows subclasses of TypedArrays to benefit from fast specialized
accesses.

Committed: https://crrev.com/6da51b4b660ea5fa594c3cb13a5ac032bbe1a1fb
Cr-Commit-Position: refs/heads/master@{#30678}

[fedor.indutny, 2015-09-09 06:25:27]

fedor@indutny.com changed reviewers:
+ machenbach@chromium.org

[fedor.indutny, 2015-09-09 06:28:32]

Hello!

This one seems to be very important for node.js, as we inherit our Buffer from
the Uint8Array.

Note that while we are doing some prototype twiddling:

https://github.com/nodejs/node/blob/f8152df5e815c98be702fe58753258b58f02c4ad/...

Buffer does not actually override neither of the accessors to the Uint8Array
properties. So it should still be optimizied into a field load.

Applying this fix improves performance in x10 times.

Thanks a lot!

Fedor.

[boopathi, 2015-09-09 07:07:10]

legend.raju@gmail.com changed reviewers:
+ legend.raju@gmail.com

[boopathi, 2015-09-09 07:07:10]

lgtm

[fedor.indutny, 2015-09-09 07:08:21]

The CQ bit was checked by fedor@indutny.com

[commit-bot: I haz the power, 2015-09-09 07:08:37]

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

[commit-bot: I haz the power, 2015-09-09 07:08:38]

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

[commit-bot: I haz the power, 2015-09-09 07:08:39]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[fedor.indutny, 2015-09-09 07:09:11]

Oops, got confused.

[Michael Achenbach, 2015-09-09 07:24:13]

machenbach@chromium.org changed reviewers:
+ adamk@chromium.org, littledan@chromium.org

[Michael Achenbach, 2015-09-09 07:24:13]

Adding better owners.

[jochen (gone - plz use gerrit), 2015-09-09 08:45:21]

jochen@chromium.org changed reviewers:
+ jkummerow@chromium.org, jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-09-09 08:45:21]

Jakob, can you take a look please?

[Jakob Kummerow, 2015-09-09 11:51:07]

The original code is broken. Instead of making it worse (uglier, hackier, more
brittle), let's fix it properly. Here's an example demonstrating failure:

var a = new Uint8Array(4);
Object.defineProperty(a, "length", {get: function() { return "blah"; }});
function getlength(x) { return x.length; }
print(getlength(a));  // correct
print(getlength(a));  // correct
print(getlength(a));  // wrong
%OptimizeFunctionOnNextCall(getlength);
print(getlength(a));  // wrong

I think what we need to do is the following:
- check if the name is one of the expected names
- perform a lookup of the corresponding property on the given map (using
LookupIterator)
- check if the holder found by the LookupIterator is identical to the original
prototype

That should fix the existing implementation for all corner cases while adding
support for arbitrary-length prototype chains and hiding low-level
implementation details as far as possible.

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:105: if (Name::Equals(name,
isolate->factory()->length_string())) {
Let's keep using the existing CheckForName helper:

if (!CheckForName(...) && !CheckForName(...) && !...) { return false; }

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:119: // %TypedArray%.prototype is non-configurable, and so are
the following
outdated comment (there are no "following named properties" any more); also it
correctly describes an incorrect implementation

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:133: // constructor
nit: trailing full stop

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:136: // If the middle-prototype does not override the fast
property -
s/ -/, then/
s/it's/its/
s/offset/offset./

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:139:
JSReceiver::GetOwnPropertyAttributes(Handle<JSObject>(js_proto),
This should use JSReceiver::HasOwnProperty(), but see overall comment

[fedor.indutny, 2015-09-09 21:20:36]

Thank you for the feedback. Hopefully, all of it was addressed.

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:105: if (Name::Equals(name,
isolate->factory()->length_string())) {
On 2015/09/09 11:51:06, Jakob wrote:
> Let's keep using the existing CheckForName helper:
> 
> if (!CheckForName(...) && !CheckForName(...) && !...) { return false; }

Acknowledged.

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:119: // %TypedArray%.prototype is non-configurable, and so are
the following
On 2015/09/09 11:51:06, Jakob wrote:
> outdated comment (there are no "following named properties" any more); also it
> correctly describes an incorrect implementation

Acknowledged.

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:133: // constructor
On 2015/09/09 11:51:07, Jakob wrote:
> nit: trailing full stop

Remove the comment.

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:136: // If the middle-prototype does not override the fast
property -
On 2015/09/09 11:51:06, Jakob wrote:
> s/ -/, then/
> s/it's/its/
> s/offset/offset./

Removed the comment, added the dot.

https://codereview.chromium.org/1313493005/diff/20001/src/accessors.cc#newcod...
src/accessors.cc:139:
JSReceiver::GetOwnPropertyAttributes(Handle<JSObject>(js_proto),
On 2015/09/09 11:51:06, Jakob wrote:
> This should use JSReceiver::HasOwnProperty(), but see overall comment

I did it in a slightly different way... Hope it is ok.

[fedor.indutny, 2015-09-09 21:24:28]

Just realized that your test still fails, because I lookup properties on the
prototype, not on the object itself. Going to change the signature of
`IsJSArrayBufferViewFieldAccessor`, or figure it out in a different way.

[fedor.indutny, 2015-09-09 22:25:14]

Jakob,

I think I have figured it out, by using the instance_descriptors. The holder
does not appear to be trivially available in hydrogen, so I wasn't able to use
it. Please let me know if this is wrong, and should be done differently.

Thank you very much for your time,
Fedor.

[Jakob Kummerow, 2015-09-10 11:33:56]

A few more minor comments. LGTM if you address those.

I've also updated the CL description to reflect the new approach.

https://codereview.chromium.org/1313493005/diff/60001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/1313493005/diff/60001/src/accessors.cc#newcod...
src/accessors.cc:113: DescriptorArray* descriptors =
map->instance_descriptors();
Before this is safe, you need to bail out for dictionary maps:

if (map->is_dictionary_map()) return false;

https://codereview.chromium.org/1313493005/diff/60001/src/accessors.cc#newcod...
src/accessors.cc:120: // Check if the property is overridden in the prototype
chain.
s/overridden/defined/

https://codereview.chromium.org/1313493005/diff/60001/src/accessors.cc#newcod...
src/accessors.cc:122: if (it.state() == LookupIterator::NOT_FOUND) return false;
if (!it.IsFound()) return false;

[fedor.indutny, 2015-09-10 11:56:29]

All fixed, thank you!

https://codereview.chromium.org/1313493005/diff/60001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/1313493005/diff/60001/src/accessors.cc#newcod...
src/accessors.cc:113: DescriptorArray* descriptors =
map->instance_descriptors();
On 2015/09/10 11:33:56, Jakob wrote:
> Before this is safe, you need to bail out for dictionary maps:
> 
> if (map->is_dictionary_map()) return false;

I knew I should do this! :) Thanks!

https://codereview.chromium.org/1313493005/diff/60001/src/accessors.cc#newcod...
src/accessors.cc:120: // Check if the property is overridden in the prototype
chain.
On 2015/09/10 11:33:56, Jakob wrote:
> s/overridden/defined/

Acknowledged.

https://codereview.chromium.org/1313493005/diff/60001/src/accessors.cc#newcod...
src/accessors.cc:122: if (it.state() == LookupIterator::NOT_FOUND) return false;
On 2015/09/10 11:33:56, Jakob wrote:
> if (!it.IsFound()) return false;

Acknowledged.

[fedor.indutny, 2015-09-10 11:56:33]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-09-10 11:56:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
legend.raju@gmail.com, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1313493005/#ps80001
(title: "last fixes")

[commit-bot: I haz the power, 2015-09-10 11:56:45]

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

[commit-bot: I haz the power, 2015-09-10 12:21:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-10 12:22:05]

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