Introduce builtin for Array.slice function.

Introduce builtin for Array.slice function.

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

[antonm, 2010-02-15 19:43:51]

Mads,

may you have a look?

Warning: it's ugly.  Let me explain the reason for current ugliness.

Alas, ECMAScript requires that ToInteger conversion of first two arguments to be
performed /after/ fetching the array (see 15.4.4.10).  I am afraid that this
conversion might perform any kind of code and thus could turn formerly fast
element JSArray into not fast element JSArray.  That's why I compile a code
which goes fast case only if both arguments passed are SMIs or undefined.

Alas, I didn't find nice mechanism to do it with the current infrastructure.  I
can see two approaches:

1) (my personal favorite): enable parameterization of builtins be generators:
into BUILTIN_LIST_C instead of second argument being a number passed into
Generator_Adaptor, let it be a generator function itself.  That would allow to
generate special code for array slice builtin.

2) extend special function info with additional flags for both compilation and
CallIC miss.

What would you recommend?

[antonm, 2010-02-16 07:06:22]

Mads,

now reworked to do type checks in the builtin itself.

May you have a look now?

[Mads Ager (chromium), 2010-02-16 09:03:03]

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

http://codereview.chromium.org/604059/diff/4001/4003#newcode431
src/builtins.cc:431: if (fast_slice == JSFunction::cast(table->get(index +
2))->code()) {
This seems like a hack.  Why not just lookup the function on the builtins
object?

Top::global_context()->builtins()->GetProperty("...");

http://codereview.chromium.org/604059/diff/4001/4003#newcode486
src/builtins.cc:486: // Step 6.
Do these steps refer to steps in the spec?  If they do, please make a complete
reference: ECMAScript 262, 3rd Edition, Section ..., step 6.

http://codereview.chromium.org/604059/diff/4001/4003#newcode499
src/builtins.cc:499: Object* result = 0;
Just declare it below: Object* result = Heap::Allocate...

[antonm, 2010-02-16 11:00:49]

Thanks a lot for comments, Mads.

Here go/lem:
http://compute3.aar:9013/golem/r3859-v8-antonm-array-slice-vs-3859-v8.html

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

http://codereview.chromium.org/604059/diff/4001/4003#newcode431
src/builtins.cc:431: if (fast_slice == JSFunction::cast(table->get(index +
2))->code()) {
On 2010/02/16 09:03:03, Mads Ager wrote:
> This seems like a hack.  Why not just lookup the function on the builtins
> object?
> 
> Top::global_context()->builtins()->GetProperty("...");

I thought it could be faster and more obvious that we call shadowed version. 
Anyway done.

http://codereview.chromium.org/604059/diff/4001/4003#newcode486
src/builtins.cc:486: // Step 6.
On 2010/02/16 09:03:03, Mads Ager wrote:
> Do these steps refer to steps in the spec?  If they do, please make a complete
> reference: ECMAScript 262, 3rd Edition, Section ..., step 6.

Done.  Could easily remove them if you don't think they add any value.

http://codereview.chromium.org/604059/diff/4001/4003#newcode499
src/builtins.cc:499: Object* result = 0;
On 2010/02/16 09:03:03, Mads Ager wrote:
> Just declare it below: Object* result = Heap::Allocate...
> 

Done.

[Mads Ager (chromium), 2010-02-16 11:32:12]

LGTM

http://codereview.chromium.org/604059/diff/3002/3004
File src/builtins.cc (right):

http://codereview.chromium.org/604059/diff/3002/3004#newcode427
src/builtins.cc:427: Object* r = Heap::LookupAsciiSymbol("ArraySlice");
Since you are using handles here already, you should use
Factory::LookupAsciiSymbol and avoid the failure check.

http://codereview.chromium.org/604059/diff/3002/3008
File test/mjsunit/array-slice.js (right):

http://codereview.chromium.org/604059/diff/3002/3008#newcode28
test/mjsunit/array-slice.js:28: // Check that shifting array of holes keeps it
as array of holes
shifting -> slicing

[antonm, 2010-02-16 12:04:04]

Thanks a lot, Mads.  Submitting.

http://codereview.chromium.org/604059/diff/3002/3004
File src/builtins.cc (right):

http://codereview.chromium.org/604059/diff/3002/3004#newcode427
src/builtins.cc:427: Object* r = Heap::LookupAsciiSymbol("ArraySlice");
On 2010/02/16 11:32:12, Mads Ager wrote:
> Since you are using handles here already, you should use
> Factory::LookupAsciiSymbol and avoid the failure check.

Done.  And I switched to GetProperty from handles.

http://codereview.chromium.org/604059/diff/3002/3008
File test/mjsunit/array-slice.js (right):

http://codereview.chromium.org/604059/diff/3002/3008#newcode28
test/mjsunit/array-slice.js:28: // Check that shifting array of holes keeps it
as array of holes
On 2010/02/16 11:32:12, Mads Ager wrote:
> shifting -> slicing

Thanks a lot, done.