Always invoke C++ ArrayPush builtin.

Always invoke C++ ArrayPush builtin.

Now this builtin checks if it should go into fast case or resort to JS ArrayPush builtin.

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

[antonm, 2010-03-01 20:09:24]

Lars,

may you have a look?

[bak, 2010-03-02 13:07:56]

I like your solution. This eliminates the need for special_function_table and
makes it easier to add new special function. 
- Please complete the change.
- Cleanup the IC code where special_function_table is used.
- Please add small benchmarks so we can see it works.
//Lars

http://codereview.chromium.org/660298/diff/4/5
File src/bootstrapper.cc (right):

http://codereview.chromium.org/660298/diff/4/5#newcode1465
src/bootstrapper.cc:1465: Handle<JSFunction>
Genesis::MakeFunction(Handle<String> name,
You should change the name of this method to be more specific.

http://codereview.chromium.org/660298/diff/4/5#newcode1502
src/bootstrapper.cc:1502: Handle<JSFunction>
old(JSFunction::cast(prototype->GetProperty(*key)));
You might want an explicit check in debug mode to ensure the original function
is there.

http://codereview.chromium.org/660298/diff/4/6
File src/builtins.cc (right):

http://codereview.chromium.org/660298/diff/4/6#newcode348
src/builtins.cc:348: if (!args.receiver()->IsJSArray()) {
It would be easier to read if you could combine the check:
   Object* rec = args.receiver();
   if (!rec->IsJSArray() || JSArray::cast(rec)->HasFastElements()) {
      // Add nice comment here.
      ... bail out...
   }

[antonm, 2010-03-03 11:36:06]

Lars,

couple of notes.

That currently introduces performance hit for Dromaeo's arrays test (<2%).  I
hope I know how to fix it:

1) introduce special generators for CallIC for some functions (putting them into
shared info)---that should allow to implement ArrayPush and ArrayPop in plain
assembler for the fastest case;
2) put functions on both hidden and normal prototypes (saves exactly the map
check we're now paying for).

Regarding benchmarks: do I get you right that you want me to add another
benchmark not into v8 itself, but in perf monitoring infrastructure?

http://codereview.chromium.org/660298/diff/4/5
File src/bootstrapper.cc (right):

http://codereview.chromium.org/660298/diff/4/5#newcode1465
src/bootstrapper.cc:1465: Handle<JSFunction>
Genesis::MakeFunction(Handle<String> name,
On 2010/03/02 13:07:56, bak wrote:
> You should change the name of this method to be more specific.

Done.  Is it fine or you want something more specific?

http://codereview.chromium.org/660298/diff/4/5#newcode1502
src/bootstrapper.cc:1502: Handle<JSFunction>
old(JSFunction::cast(prototype->GetProperty(*key)));
On 2010/03/02 13:07:56, bak wrote:
> You might want an explicit check in debug mode to ensure the original function
> is there.
> 

I didn't find concise way to do that.  My plan was to fetch property from
Top::global_context()->builtins(), but the problem is methods and builtins names
are different: push vs. ArrayPush etc.  Of course, it's easy to make one from
another, but check becomes too heavyweight imho.

Still, if you think we'd better have one, I'll implement it.

Another approach might be to expose runtime function which accepts builtin name
and returns C++ builtin for this name.  In this approach array.js prototype
initialization code would look like:

  "join": ArrayJoin,
  ...
  "push": %CppBuiltin("ArrayPush"),

which is easier to comprehend for me.

http://codereview.chromium.org/660298/diff/4/6
File src/builtins.cc (right):

http://codereview.chromium.org/660298/diff/4/6#newcode348
src/builtins.cc:348: if (!args.receiver()->IsJSArray()) {
On 2010/03/02 13:07:56, bak wrote:
> It would be easier to read if you could combine the check:
>    Object* rec = args.receiver();
>    if (!rec->IsJSArray() || JSArray::cast(rec)->HasFastElements()) {
>       // Add nice comment here.
>       ... bail out...
>    }  

Done.

[antonm, 2010-03-04 13:38:29]

Lars, if you're curious.

http://codereview.chromium.org/669061/show

should compensate for the lost.

Would you prefer if I rebase this change to ToT---there has been a lot of
activity around.

[Mads Ager (chromium), 2010-03-08 11:53:00]

I like it.  I would like to understand why the method to call the special
functions is called at a different time on ARM before putting this in. Other
than that it LGTM.

http://codereview.chromium.org/660298/diff/33/1027
File src/bootstrapper.cc (right):

http://codereview.chromium.org/660298/diff/33/1027#newcode1477
src/bootstrapper.cc:1477: Object* old_value = prototype->GetProperty(*key);
Since everything else is in handles here, could you use the handle-based version
of GetProperty as well?

Handle<Object> old_value = GetProperty(prototype, key);

http://codereview.chromium.org/660298/diff/33/1027#newcode1478
src/bootstrapper.cc:1478: // On ARM we sometimes call this method before
installing natives.
But we do call it after installing natives as well, right?  Why do we call it
before installing natives on ARM and not on the other platforms? I find that a
little puzzling.

http://codereview.chromium.org/660298/diff/33/1028
File src/builtins.cc (right):

http://codereview.chromium.org/660298/diff/33/1028#newcode322
src/builtins.cc:322: class FastElementsArrayCheck {
I would make this a static method 'IsJSArrayWithFastElements' with a bool result
type and a couple of out parameters.

[antonm, 2010-03-09 10:04:29]

Thanks a lot for comments, Mads.  Hopefully all of them are responded.

http://codereview.chromium.org/660298/diff/33/1027
File src/bootstrapper.cc (right):

http://codereview.chromium.org/660298/diff/33/1027#newcode1477
src/bootstrapper.cc:1477: Object* old_value = prototype->GetProperty(*key);
On 2010/03/08 11:53:01, Mads Ager wrote:
> Since everything else is in handles here, could you use the handle-based
version
> of GetProperty as well?
> 
> Handle<Object> old_value = GetProperty(prototype, key); 

Done.

http://codereview.chromium.org/660298/diff/33/1027#newcode1478
src/bootstrapper.cc:1478: // On ARM we sometimes call this method before
installing natives.
On 2010/03/08 11:53:01, Mads Ager wrote:
> But we do call it after installing natives as well, right?  Why do we call it
> before installing natives on ARM and not on the other platforms? I find that a
> little puzzling.

Sorry, comment was somewhat misleading, I've tried to adjust it.

The story: there are test/cctest/test-assembler-arm tests which set
FLAG_natives_file to empty string thus bypassing JS natives installation. 
However that's apparently just an example of more common problem if someone for
some strange reason overrides natives installation.

BTW, I wonder if InstallNatives should return true in this case.

http://codereview.chromium.org/660298/diff/33/1028
File src/builtins.cc (right):

http://codereview.chromium.org/660298/diff/33/1028#newcode322
src/builtins.cc:322: class FastElementsArrayCheck {
On 2010/03/08 11:53:01, Mads Ager wrote:
> I would make this a static method 'IsJSArrayWithFastElements' with a bool
result
> type and a couple of out parameters.

Done.

[Mads Ager (chromium), 2010-03-09 11:14:56]

LGTM