Faster invocation of custom comparator function.

Faster invocation of custom comparator function.

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

[antonm, 2010-04-08 11:12:22]

Mads, Vitaly,

may you have a look?

And many thanks to Vitaly for discussion.

[Vitaly Repeshko, 2010-04-08 13:09:16]

LG, but is EnsureCompiled really required? If yes, we should probably have an
assert somewhere. If not, let's get rid of it.


-- Vitaly

http://codereview.chromium.org/1623004/diff/1/4
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1623004/diff/1/4#newcode6137
src/ia32/codegen-ia32.cc:6137: void
CodeGenerator::GenerateCallFunction(ZoneList<Expression*>* args) {
You can make it support variable number of arguments by requiring one of them to
be constant smi "argc".

[Mads Ager (chromium), 2010-04-08 13:19:58]

LGTM

http://codereview.chromium.org/1623004/diff/1/4
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1623004/diff/1/4#newcode6137
src/ia32/codegen-ia32.cc:6137: void
CodeGenerator::GenerateCallFunction(ZoneList<Expression*>* args) {
On 2010/04/08 13:09:16, Vitaly wrote:
> You can make it support variable number of arguments by requiring one of them
to
> be constant smi "argc".

I agree, since is is just called CallFunction it would be nice to do the easy
generalization to more arguments.

http://codereview.chromium.org/1623004/diff/1/8
File src/runtime.cc (right):

http://codereview.chromium.org/1623004/diff/1/8#newcode1367
src/runtime.cc:1367: return Top::context()->global()->global_context()
This looks weird. You should add a comment stating how this this gets you to the
actual global receiver instead of the builtins object.  Maybe you can make it
easier to read if you name Top::context()->global() first and then go through
the global context to get the real global and global receiver?

[antonm, 2010-04-08 15:03:46]

Fast response in hope you'll answer today.

Regarding varagrs for CallFunction.  Technically I think I don't need smi as I
have args.length(), the problem is how to declare such a function in codegen.h? 
Do you know for sure?  I am going to declare it with minimal possible number of
args (2, receiver and func).  Any better ideas?

@Vitaly: yes, EnsureCompiled is unnecessary, thanks a lot!

Removing it and porting to other platforms.  Will send next patch for review
soon.

Thanks for all your comments.

[antonm, 2010-04-08 16:28:14]

1) ported to x64 and ARM, should I make stubs for mips?
2) CallFunction is now variardic.
2) I've dropped saving context before calling function (it should be noop,
shouldn't it)

May you have another look?

http://codereview.chromium.org/1623004/diff/1/4
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1623004/diff/1/4#newcode6137
src/ia32/codegen-ia32.cc:6137: void
CodeGenerator::GenerateCallFunction(ZoneList<Expression*>* args) {
On 2010/04/08 13:19:58, Mads Ager wrote:
> On 2010/04/08 13:09:16, Vitaly wrote:
> > You can make it support variable number of arguments by requiring one of
them
> to
> > be constant smi "argc".
> 
> I agree, since is is just called CallFunction it would be nice to do the easy
> generalization to more arguments.
> 

Done.

http://codereview.chromium.org/1623004/diff/1/8
File src/runtime.cc (right):

http://codereview.chromium.org/1623004/diff/1/8#newcode1367
src/runtime.cc:1367: return Top::context()->global()->global_context()
On 2010/04/08 13:19:58, Mads Ager wrote:
> This looks weird. You should add a comment stating how this this gets you to
the
> actual global receiver instead of the builtins object.  Maybe you can make it
> easier to read if you name Top::context()->global() first and then go through
> the global context to get the real global and global receiver?

Done.  But I'd appreciate if you could check the wording.

[Vitaly Repeshko, 2010-04-08 16:45:05]

LGTM.

[Mads Ager (chromium), 2010-04-09 08:13:35]

LGTM