Implement efficient element copying in ElementsAccessors.

Implement efficient element copying in ElementsAccessors.

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

[danno, 2012-03-08 17:25:11]

PTAL.

Sven, can you take a look at the template usage. Jakob, please take a look at
the element accessor changes.

Review guidance:

- Start in elements.h. There are new virtual methods on ElementsAccessor  for
copying elements from on elements backing store to another. 
- In elements.cc, a Traits class has been used that maps an ElementsKind to its
backing store are compile time. The changes in the macros allow the traits
classes to be generated automatically. The bulk of the changes are the
implementation of CopyElementsImpl in the elements specialization classes, which
call specialized copiers.
- builtins.cc new uses the common object-to-object copying method that is also
used by the ElementAccessor

[Sven Panne, 2012-03-09 08:15:18]

LGTM with nits. Hairy stuff... o_O

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode610
src/elements.cc:610: static MaybeObject* CopyElementsImpl(FixedArrayBase* from,
This could return Object*, I think, allocations are not allowed anyway. OTOH, my
mental compiler might overlook places where an actual failure could arise...

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode989
src/elements.cc:989: return from;      default:
Layout...

http://codereview.chromium.org/9638014/diff/3005/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/9638014/diff/3005/src/elements.h#newcode97
src/elements.h:97: FixedArrayBase* from = NULL) = 0;
Reading only this header, it is a bit confusing why there is "from" in addition
to "from_holder". A comment for a method with more than half a dozen parameter
wouldn't hurt... ;-)

http://codereview.chromium.org/9638014/diff/3005/src/elements.h#newcode145
src/elements.h:145: void CopyObjectToObjectElements(AssertNoAllocation* no_gc,
Hmmm, the first parameter is actually a phantom type, I guess. We have one other
such type in v8 I know of (WhitenessWitness), but I have the gut feeling that
passing an instance of such phantom types around at runtime is very inelegant.
What we really want is a compile time check with no runtime overhead. I am sure
one can solve this via template tricks^H^H^H technology, but I don't have a
solution at hand. Not a blocker for the CL, but just something to keep in mind.

[Jakob Kummerow, 2012-03-09 10:09:33]

LGTM with comments.

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc
File src/elements.cc (right):

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newc...
src/elements.cc:65: // identical.  Note that the order must match that of the
ElementsKind enum for
It sure would be nice to enforce this matching order, e.g. by using this macro
to generate the enum... but I realize that header dependencies might make that
difficult.

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newc...
src/elements.cc:148: if (copy_size == -1) copy_size = from_obj->length() -
from_start;
I'd move this line up so that the auto-detected copy_size is still covered by
the ASSERT((copy_size + static_cast<int>(from_start)) <= from_obj->length()).
As an additional sanity check, you could add ASSERT(copy_size >= 0), but I guess
it's unlikely that would ever be triggered.

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newc...
src/elements.cc:209: if (copy_size == -1) copy_size = from_obj->length() -
from_start;
I'd move this up too (see my other comment above).

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newc...
src/elements.cc:228: CHECK(maybe_value_object->ToObject(&value));
What if the old space needs a GC? I think you want to return the failure object
in this case instead of just CHECKing it.

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newc...
src/elements.cc:245: if (copy_size == -1) copy_size = from_obj->length() -
from_start;
And once more, I'd move this up.

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newc...
src/elements.cc:695: to_start, copy_size);
nit: indentation

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newc...
src/elements.cc:989: return from;      default:
nit: new line for "default:" please.

https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newc...
src/elements.cc:1052: NonStrictArgumentsElementsAccessor,
very nitty nit: 4 spaces indent, not 5 :-)

[danno, 2012-03-09 12:12:35]

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode65
src/elements.cc:65: // identical.  Note that the order must match that of the
ElementsKind enum for
I think it would be nice, but it's a bit beyond the scope of this CL. 
On 2012/03/09 10:09:36, Jakob wrote:
> It sure would be nice to enforce this matching order, e.g. by using this macro
> to generate the enum... but I realize that header dependencies might make that
> difficult.

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode148
src/elements.cc:148: if (copy_size == -1) copy_size = from_obj->length() -
from_start;
On 2012/03/09 10:09:36, Jakob wrote:
> I'd move this line up so that the auto-detected copy_size is still covered by
> the ASSERT((copy_size + static_cast<int>(from_start)) <= from_obj->length()).
> As an additional sanity check, you could add ASSERT(copy_size >= 0), but I
guess
> it's unlikely that would ever be triggered.

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode209
src/elements.cc:209: if (copy_size == -1) copy_size = from_obj->length() -
from_start;
On 2012/03/09 10:09:36, Jakob wrote:
> I'd move this up too (see my other comment above).

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode228
src/elements.cc:228: CHECK(maybe_value_object->ToObject(&value));
On 2012/03/09 10:09:36, Jakob wrote:
> What if the old space needs a GC? I think you want to return the failure
object
> in this case instead of just CHECKing it.

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode245
src/elements.cc:245: if (copy_size == -1) copy_size = from_obj->length() -
from_start;
On 2012/03/09 10:09:36, Jakob wrote:
> And once more, I'd move this up.

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode610
src/elements.cc:610: static MaybeObject* CopyElementsImpl(FixedArrayBase* from,
The allocation of HeapNumbers can fail on the Double -> Fast conversion, so this
has to be MaybeObject. 
On 2012/03/09 08:15:18, Sven Panne wrote:
> This could return Object*, I think, allocations are not allowed anyway. OTOH,
my
> mental compiler might overlook places where an actual failure could arise...

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode695
src/elements.cc:695: to_start, copy_size);
On 2012/03/09 10:09:36, Jakob wrote:
> nit: indentation

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode989
src/elements.cc:989: return from;      default:
On 2012/03/09 08:15:18, Sven Panne wrote:
> Layout...

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode989
src/elements.cc:989: return from;      default:
On 2012/03/09 08:15:18, Sven Panne wrote:
> Layout...

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode1052
src/elements.cc:1052: NonStrictArgumentsElementsAccessor,
On 2012/03/09 10:09:36, Jakob wrote:
> very nitty nit: 4 spaces indent, not 5 :-)

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/9638014/diff/3005/src/elements.h#newcode97
src/elements.h:97: FixedArrayBase* from = NULL) = 0;
On 2012/03/09 08:15:18, Sven Panne wrote:
> Reading only this header, it is a bit confusing why there is "from" in
addition
> to "from_holder". A comment for a method with more than half a dozen parameter
> wouldn't hurt... ;-)

Done.

http://codereview.chromium.org/9638014/diff/3005/src/elements.h#newcode145
src/elements.h:145: void CopyObjectToObjectElements(AssertNoAllocation* no_gc,
On 2012/03/09 08:15:18, Sven Panne wrote:
> Hmmm, the first parameter is actually a phantom type, I guess. We have one
other
> such type in v8 I know of (WhitenessWitness), but I have the gut feeling that
> passing an instance of such phantom types around at runtime is very inelegant.
> What we really want is a compile time check with no runtime overhead. I am
sure
> one can solve this via template tricks^H^H^H technology, but I don't have a
> solution at hand. Not a blocker for the CL, but just something to keep in
mind.

Done.

[Michael Starzinger, 2012-03-09 12:34:32]

LGTM (with one bug). I just looked at the write-barrier related stuff.

https://chromiumcodereview.appspot.com/9638014/diff/9003/src/elements.cc
File src/elements.cc (right):

https://chromiumcodereview.appspot.com/9638014/diff/9003/src/elements.cc#newc...
src/elements.cc:222: if (!maybe_value->ToObject(&value)) {
If you are sure that this can only fail because of an allocation failure, I
would add an assertion like the following here.

ASSERT(maybe_value->IsRetryAfterGC() || maybe_value->IsOutOfMemory());

https://chromiumcodereview.appspot.com/9638014/diff/9003/src/elements.cc#newc...
src/elements.cc:225: heap->AllocateHeapNumber(from_obj->get_scalar(i), TENURED);
Shouldn't this be "get_scalar(i + from_start)" here?

[danno, 2012-03-09 13:21:06]

Feedback addressed, landing

http://codereview.chromium.org/9638014/diff/9003/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/9638014/diff/9003/src/elements.cc#newcode222
src/elements.cc:222: if (!maybe_value->ToObject(&value)) {
On 2012/03/09 12:34:32, Michael Starzinger wrote:
> If you are sure that this can only fail because of an allocation failure, I
> would add an assertion like the following here.
> 
> ASSERT(maybe_value->IsRetryAfterGC() || maybe_value->IsOutOfMemory());

Done.

http://codereview.chromium.org/9638014/diff/9003/src/elements.cc#newcode225
src/elements.cc:225: heap->AllocateHeapNumber(from_obj->get_scalar(i), TENURED);
On 2012/03/09 12:34:32, Michael Starzinger wrote:
> Shouldn't this be "get_scalar(i + from_start)" here?

Done.