Add Array.concat builtin for the most common case.

Add Array.concat builtin for the most common case.

Committed: http://code.google.com/p/v8/source/detail?r=4129

[antonm, 2010-03-15 07:53:11]

Lasse, Mads,

may you have a look?

I am aware of ArrayConcat in runtime, but builtins are somewhat faster.

[Mads Ager (chromium), 2010-03-15 08:17:00]

LGTM

[Lasse Reichstein, 2010-03-15 11:45:41]

LGTM

http://codereview.chromium.org/954001/diff/4001/5002
File src/builtins.cc (right):

http://codereview.chromium.org/954001/diff/4001/5002#newcode742
src/builtins.cc:742: if (!IsJSArrayWithFastElements(receiver_obj,
&receiver_elms)
I personally don't like test functions with side effects. Could you just do the
assignments after the if-statement instead?

http://codereview.chromium.org/954001/diff/4001/5002#newcode755
src/builtins.cc:755: ASSERT(receiver_len <= (Smi::kMaxValue - arg_len));
Maybe comment that this assertion holds because both arrays have fast elements
(possibly rewriting it in terms of FixedArray::kMaxLength).

http://codereview.chromium.org/954001/diff/4001/5002#newcode758
src/builtins.cc:758: if (result_len > FixedArray::kMaxSize) {
FixedArray::kMaxLength (length is in elements, size is in bytes).

[antonm, 2010-03-15 14:53:11]

Lasse, sorry, I've submitted the CL already.  Sending another CL to you.

http://codereview.chromium.org/954001/diff/4001/5002
File src/builtins.cc (right):

http://codereview.chromium.org/954001/diff/4001/5002#newcode742
src/builtins.cc:742: if (!IsJSArrayWithFastElements(receiver_obj,
&receiver_elms)
On 2010/03/15 11:45:41, Lasse Reichstein wrote:
> I personally don't like test functions with side effects. Could you just do
the
> assignments after the if-statement instead?
> 

It used to save some cycles, at least in previous versions.  If you like, I can
run another experiment to see how much (if anything) does it buy us now.

http://codereview.chromium.org/954001/diff/4001/5002#newcode755
src/builtins.cc:755: ASSERT(receiver_len <= (Smi::kMaxValue - arg_len));
On 2010/03/15 11:45:41, Lasse Reichstein wrote:
> Maybe comment that this assertion holds because both arrays have fast elements
> (possibly rewriting it in terms of FixedArray::kMaxLength).

Done

http://codereview.chromium.org/954001/diff/4001/5002#newcode758
src/builtins.cc:758: if (result_len > FixedArray::kMaxSize) {
On 2010/03/15 11:45:41, Lasse Reichstein wrote:
> FixedArray::kMaxLength (length is in elements, size is in bytes).

Oh yes, thanks a lot for spotting this.