Use InternalArray in Object.getOwnPropertyNames() implementation

Use InternalArray in Object.getOwnPropertyNames() implementation

Committed: https://code.google.com/p/v8/source/detail?r=13918

[Vyacheslav Egorov (Google), 2013-02-26 01:21:32]

I defer to Michael for final opinion.

https://codereview.chromium.org/12342003/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/12342003/diff/1/src/array.js#newcode1556
src/array.js:1556: "concat", getFunction("concat", ArrayConcat),
concat will return a normal Array, thus if you are doing several concats in a
row then only the first one is protected, the rest can be replaced.

(can happen in interceptor case).

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js#newcode1032
src/v8natives.js:1032: propertyNames =
propertyNames.concat(indexedInterceptorNames);
I guess here you should %MoveArrayContents instead of assigning

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js#newcode1039
src/v8natives.js:1039: propertyNames =
propertyNames.concat(%GetLocalPropertyNames(obj));
ditto

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js#newcode1046
src/v8natives.js:1046: propertyNames =
propertyNames.concat(namedInterceptorNames);
here I guess you can leave assignment. though for consistency you might want to
%Move...

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js#newcode1057
src/v8natives.js:1057: // We need to check for the exact property value since
for intrinsic
I think this comment is out of date

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js#newcode1069
src/v8natives.js:1069: var result = [];
I wonder if it should be new $Array() for consistency with some other places
(e.g. map).

[adamk, 2013-02-26 01:44:53]

https://codereview.chromium.org/12342003/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/12342003/diff/1/src/array.js#newcode1556
src/array.js:1556: "concat", getFunction("concat", ArrayConcat),
On 2013/02/26 01:21:33, Vyacheslav Egorov (Google) wrote:
> concat will return a normal Array, thus if you are doing several concats in a
> row then only the first one is protected, the rest can be replaced.
> 
> (can happen in interceptor case).

I think this is enough reason to take another approach here; Michael, you can
hold off on reviewing until I ping this back again.

[adamk, 2013-02-26 02:28:12]

Okay, updated with two changes:

- InternalArray.prototype.concat now returns an InternalArray
- Only call concat once

Let me know if I've gone too far overboard with re-engineering this. Especially
since it's still susceptible to being messed with by the embedder if they return
a sparse array from the interceptor property enumerators.

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js#newcode1057
src/v8natives.js:1057: // We need to check for the exact property value since
for intrinsic
On 2013/02/26 01:21:33, Vyacheslav Egorov (Google) wrote:
> I think this comment is out of date

Happy to remove if mstarzinger agrees.

https://codereview.chromium.org/12342003/diff/1/src/v8natives.js#newcode1069
src/v8natives.js:1069: var result = [];
On 2013/02/26 01:21:33, Vyacheslav Egorov (Google) wrote:
> I wonder if it should be new $Array() for consistency with some other places
> (e.g. map).

Changed to $Array.

[adamk, 2013-03-05 16:23:25]

Michael, can you take a look at this?

[Michael Starzinger, 2013-03-11 12:02:38]

Sorry for the long delay. This CL completely fell through the cracks somehow.
Now it is on my radar though. :)

This goes through lengths to install concat() on the prototype of InternalArray
just to call it through the internal %Apply() function anyways. Can we get by
without installing that function on the InternalArray prototype at all?

[adamk, 2013-03-11 16:13:11]

On 2013/03/11 12:02:38, Michael Starzinger wrote:
> Sorry for the long delay. This CL completely fell through the cracks somehow.
> Now it is on my radar though. :)
> 
> This goes through lengths to install concat() on the prototype of
InternalArray
> just to call it through the internal %Apply() function anyways. Can we get by
> without installing that function on the InternalArray prototype at all?

Yeah, I realize that's sorta weird at this point.  The code's gone through a few
reconfigurations, and I realized at some point that I could reduce the number of
concat()s necessary by using %Apply (previously I was calling concat() directly
on instances). It's not clear what a better place for the concat method would
be. Just some property on the builtin's global?

[Michael Starzinger, 2013-03-11 17:14:18]

Since ObjectGetOwnPropertyNames() is the only call-site that requires
ArrayConcat and then copies the result into an InternalArray, I would just go
with inlining all of that into ObjectGetOwnPropertyNames() itself. That would be
the easiest solution until we need a more general concat functionality on
InternalArray.

Also after the point that concat is called in that function, no further
prototype functions are called on the resulting array. So IMHO it is safe to use
an unwrapped concat here, or am I missing something?

[adamk, 2013-03-11 17:30:25]

On 2013/03/11 17:14:18, Michael Starzinger wrote:
> Since ObjectGetOwnPropertyNames() is the only call-site that requires
> ArrayConcat and then copies the result into an InternalArray, I would just go
> with inlining all of that into ObjectGetOwnPropertyNames() itself. That would
be
> the easiest solution until we need a more general concat functionality on
> InternalArray.

The tricky part is getting ahold of the version of ArrayConcat from builtins.cc.
SetUpArray() has access to those (through its call to %SpecialArrayFunctions),
but I'd hate to have to re-run that code (or even a subset of it) inside
GetOwnPropertyNames. It seemed to me that the most reasonable way to expose that
builtin was through InternalArray. Do you have a particular concern with making
InternalArray more capable?

> Also after the point that concat is called in that function, no further
> prototype functions are called on the resulting array. So IMHO it is safe to
use
> an unwrapped concat here, or am I missing something?

You're right than an unwrapped concat is fine here for now, though I worry a bit
about someone adding something fancy to that interceptor de-duping loop. But
this is a bit of a separate point from that of getting access to the proper
array builtin for concat() discussed above.

[Michael Starzinger, 2013-03-12 12:06:12]

I don't have a general concern about making InternalArray more capable. But the
getInternalArrayConcat() function just feels bolted on. So unless you have other
use-cases for InternalArray.prototype.concat in mind that requires it to return
an InternalArray instead of an Array, I would just add the unmodified concat
function to the prototype. That achieves the same thing (now that you reworked
this CL after Slava's comments), even without redundantly moving the array
content twice.

I am just somewhat hesitant to apply a "slightly more general but band-aid
solution" to a problem that we don't yet have. Also
InternlArray.prototype.splice suffers from the same problem, it also returns an
Array as opposed to an InternalArray. But as I said, all of that only applies if
there are no other follow-up CLs that build on top of this change that I am
missing.

[Michael Starzinger, 2013-03-12 12:09:06]

One round of actionable comments. Sorry for being so vague before.

https://codereview.chromium.org/12342003/diff/5001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/12342003/diff/5001/src/array.js#newcode1554
src/array.js:1554: var getInternalArrayConcat = function(concatFn) {
Drop this wrapper generator for a wrapper function.

https://codereview.chromium.org/12342003/diff/5001/src/array.js#newcode1567
src/array.js:1567: "concat", getInternalArrayConcat(getFunction("concat",
ArrayConcat)),
Just install the plain unwrapped ArrayConcat functionality here.

https://codereview.chromium.org/12342003/diff/5001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/12342003/diff/5001/src/v8natives.js#newcode1076
src/v8natives.js:1076: %MoveArrayContents(propertyNames, result);
This call to MoveArrayContents has become obsolete. Just return propertyNames
here.

[adamk, 2013-03-12 18:25:18]

Had to rebaseline; uploaded a patch in the middle (3) so you can diff against
that.

I've taken care of the requested changes. Seems like we might want to come up
with a better concat in the future, but I agree it's reasonable to just do this
for now.

https://codereview.chromium.org/12342003/diff/5001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/12342003/diff/5001/src/array.js#newcode1554
src/array.js:1554: var getInternalArrayConcat = function(concatFn) {
On 2013/03/12 12:09:06, Michael Starzinger wrote:
> Drop this wrapper generator for a wrapper function.

Done.

https://codereview.chromium.org/12342003/diff/5001/src/array.js#newcode1567
src/array.js:1567: "concat", getInternalArrayConcat(getFunction("concat",
ArrayConcat)),
On 2013/03/12 12:09:06, Michael Starzinger wrote:
> Just install the plain unwrapped ArrayConcat functionality here.

Done.

https://codereview.chromium.org/12342003/diff/5001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/12342003/diff/5001/src/v8natives.js#newcode1076
src/v8natives.js:1076: %MoveArrayContents(propertyNames, result);
On 2013/03/12 12:09:06, Michael Starzinger wrote:
> This call to MoveArrayContents has become obsolete. Just return propertyNames
> here.

Done.

[Michael Starzinger, 2013-03-12 19:46:49]

LGTM.