Move CopyElements to the accessor of the target.

Move CopyElements to the accessor of the target.

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

[Toon Verwaest, 2012-11-28 13:30:05]

PTAL

[Michael Starzinger, 2012-11-29 09:39:32]

First round.

https://codereview.chromium.org/11416238/diff/1001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/11416238/diff/1001/src/builtins.cc#newcode1227
src/builtins.cc:1227: ElementsKind origin_kind = array->GetElementsKind();
Use "from_kind" as name for consistency.

https://codereview.chromium.org/11416238/diff/1001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11416238/diff/1001/src/elements.cc#newcode1045
src/elements.cc:1045: return CopyElementsImpl(arguments, from_start, to,
from_kind,
Are you sure this is correct? This seems to only copy elements from the
arguments backing store but ignore aliased arguments. Also it seems like the
AliasedArgumentsEntry might escape through this path.

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

https://codereview.chromium.org/11416238/diff/1001/src/objects-inl.h#newcode1749
src/objects-inl.h:1749: ElementsKind FixedArrayBase::GetElementsKind() {
I don't like moving this outside of elements accessors. The only one who should
be concerned about the elements kind for a certain backing store are elements
accessors. That's why it belongs into that class. Please move it back.

https://codereview.chromium.org/11416238/diff/1001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11416238/diff/1001/src/objects.h#newcode2338
src/objects.h:2338: inline ElementsKind GetElementsKind();
See comments in objects-inl.h about this method.

[Michael Starzinger, 2012-11-29 10:17:45]

https://codereview.chromium.org/11416238/diff/1001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11416238/diff/1001/src/elements.cc#newcode1045
src/elements.cc:1045: return CopyElementsImpl(arguments, from_start, to,
from_kind,
On 2012/11/29 09:39:32, Michael Starzinger wrote:
> Are you sure this is correct? This seems to only copy elements from the
> arguments backing store but ignore aliased arguments. Also it seems like the
> AliasedArgumentsEntry might escape through this path.

I looked into this problem. The current situation is horrible. The only way I
see to resolve this cleanly is to move the implementation of
SetFastElementsCapacityAndLength from objects.cc into the elements accessor
next. Thereby not calling CopyElements() on arguments->elements() with
arguments->elements()->get(1) as the target, which is just insane.

Could you please add a TODO to this case that it should actually be unreachable
and is a temporary hack for SetFastElementsCapacityAndLength() only?

[Toon Verwaest, 2012-11-29 14:04:40]

Addressed comments.

https://chromiumcodereview.appspot.com/11416238/diff/1001/src/builtins.cc
File src/builtins.cc (right):

https://chromiumcodereview.appspot.com/11416238/diff/1001/src/builtins.cc#new...
src/builtins.cc:1227: ElementsKind origin_kind = array->GetElementsKind();
On 2012/11/29 09:39:32, Michael Starzinger wrote:
> Use "from_kind" as name for consistency.

Done.

https://chromiumcodereview.appspot.com/11416238/diff/1001/src/elements.cc
File src/elements.cc (right):

https://chromiumcodereview.appspot.com/11416238/diff/1001/src/elements.cc#new...
src/elements.cc:1045: return CopyElementsImpl(arguments, from_start, to,
from_kind,
On 2012/11/29 10:17:45, Michael Starzinger wrote:
> On 2012/11/29 09:39:32, Michael Starzinger wrote:
> > Are you sure this is correct? This seems to only copy elements from the
> > arguments backing store but ignore aliased arguments. Also it seems like the
> > AliasedArgumentsEntry might escape through this path.
> 
> I looked into this problem. The current situation is horrible. The only way I
> see to resolve this cleanly is to move the implementation of
> SetFastElementsCapacityAndLength from objects.cc into the elements accessor
> next. Thereby not calling CopyElements() on arguments->elements() with
> arguments->elements()->get(1) as the target, which is just insane.
> 
> Could you please add a TODO to this case that it should actually be
unreachable
> and is a temporary hack for SetFastElementsCapacityAndLength() only?

Done.

https://chromiumcodereview.appspot.com/11416238/diff/1001/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/11416238/diff/1001/src/objects-inl.h#n...
src/objects-inl.h:1749: ElementsKind FixedArrayBase::GetElementsKind() {
On 2012/11/29 09:39:32, Michael Starzinger wrote:
> I don't like moving this outside of elements accessors. The only one who
should
> be concerned about the elements kind for a certain backing store are elements
> accessors. That's why it belongs into that class. Please move it back.

Done.

https://chromiumcodereview.appspot.com/11416238/diff/1001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11416238/diff/1001/src/objects.h#newco...
src/objects.h:2338: inline ElementsKind GetElementsKind();
On 2012/11/29 09:39:32, Michael Starzinger wrote:
> See comments in objects-inl.h about this method.

Done.

[danno, 2012-11-29 15:42:28]

Looks like you have another patch erroneously included with this one. Can you
re-separate?

https://codereview.chromium.org/11416238/diff/8002/src/elements.h
File src/elements.h (right):

https://codereview.chromium.org/11416238/diff/8002/src/elements.h#newcode147
src/elements.h:147: ElementsKind from_kind,
from_kind -> source_kind, perhaps group with other "source" parameters?

[Toon Verwaest, 2012-11-29 16:30:14]

Addressed comment. PTAL.

[Michael Starzinger, 2012-11-29 17:13:16]

LGTM. But I would wait for Danno's review on this as well.

[danno, 2012-12-11 17:07:04]

LGTM if you address comments

https://codereview.chromium.org/11416238/diff/10002/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11416238/diff/10002/src/elements.cc#newcode1005
src/elements.cc:1005: static ElementsKind ElementsKindForArray(FixedArrayBase*
array) {
Not sure how performance sensitive this is, but how about forcing this to
inline, too?

https://codereview.chromium.org/11416238/diff/10002/src/elements.cc#newcode1014
src/elements.cc:1014: return FAST_HOLEY_DOUBLE_ELEMENTS;
How about a lookup array instead of the switch for the FIXED_ARRAY_TYPE case.
Again, not sure if this is super performance critical, but it could be for
copies of small site.

https://codereview.chromium.org/11416238/diff/10002/src/elements.cc#newcode1079
src/elements.cc:1079: // This case should be UNREACHABLE().
Yikes, this is totally scary.

[Toon Verwaest, 2012-12-14 12:39:12]

https://codereview.chromium.org/11416238/diff/10002/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11416238/diff/10002/src/elements.cc#newcode1005
src/elements.cc:1005: static ElementsKind ElementsKindForArray(FixedArrayBase*
array) {
On 2012/12/11 17:07:04, danno wrote:
> Not sure how performance sensitive this is, but how about forcing this to
> inline, too?

Done.

https://codereview.chromium.org/11416238/diff/10002/src/elements.cc#newcode1014
src/elements.cc:1014: return FAST_HOLEY_DOUBLE_ELEMENTS;
The problem is that this switches over instances types. There are very many of
those, so most aren't handled here. We could shift and only look at the window
of used types, but even those are not consecutive (at least FIXED_ARRAY_TYPE
isn't in the same range as the others).

On 2012/12/11 17:07:04, danno wrote:
> How about a lookup array instead of the switch for the FIXED_ARRAY_TYPE case.
> Again, not sure if this is super performance critical, but it could be for
> copies of small site.

https://codereview.chromium.org/11416238/diff/10002/src/elements.cc#newcode1079
src/elements.cc:1079: // This case should be UNREACHABLE().
Indeed. This should be cleaned up sometime; but that's unrelated to this CL...

On 2012/12/11 17:07:04, danno wrote:
> Yikes, this is totally scary.

[danno, 2012-12-18 12:24:54]

still lgtm