Simplify passing of AccessorInfo to interceptors:

Simplify passing of AccessorInfo to interceptors:

 * Use slots on the native stack when possible instead of Relocatable.
 * Got rid of a gap in AccessorInfo fields.
 * Added test for non-cacheable post-interceptor lookup.

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

[Mads Ager (chromium), 2010-03-25 14:33:00]

LGTM

Are CustomArguments still used elsewhere or can we remove them completely?

[antonm, 2010-03-25 14:40:53]

LGTM

http://codereview.chromium.org/1327002/diff/1/3
File src/arguments.h (right):

http://codereview.chromium.org/1327002/diff/1/3#newcode75
src/arguments.h:75: // Cursom arguments replicate a small segment of stack that
can be
could you fix it while you are around, please?  Cursom -> Custom.

http://codereview.chromium.org/1327002/diff/1/3#newcode90
src/arguments.h:90: Object* values_[3];
should we introduce constants to minimize changes?  by no means insisting.

http://codereview.chromium.org/1327002/diff/1/8
File src/stub-cache.cc (right):

http://codereview.chromium.org/1327002/diff/1/8#newcode806
src/stub-cache.cc:806: v8::AccessorInfo info(args.arguments() - 2);
maybe add a special method/constructor?  this 2 looks too magical imho

http://codereview.chromium.org/1327002/diff/3001/4002
File src/arguments.h (right):

http://codereview.chromium.org/1327002/diff/3001/4002#newcode78
src/arguments.h:78: class CustomArguments : public Relocatable {
do we still use CustomArguments?

http://codereview.chromium.org/1327002/diff/3001/4006
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/1327002/diff/3001/4006#newcode1043
src/ia32/stub-cache-ia32.cc:1043: __ mov(other, esp);
Do we need to push the address?

http://codereview.chromium.org/1327002/diff/3001/4006#newcode1054
src/ia32/stub-cache-ia32.cc:1054: __ add(Operand(eax), Immediate(4 *
kPointerSize));
could we use some named constant for this 4?

http://codereview.chromium.org/1327002/diff/3001/4006#newcode1058
src/ia32/stub-cache-ia32.cc:1058: ASSERT_EQ(5, ApiGetterEntryStub::kStackSpace);
do you I understand it right, this is not ported to other platforms?  do you
know if we have corresponding bugs filed?

[Vitaly Repeshko, 2010-03-25 17:06:46]

On 2010/03/25 14:33:00, Mads Ager wrote:
> Are CustomArguments still used elsewhere or can we remove them completely?

They are still used. When can get rid of them in some places, but some other
places are harder.


Thanks,
Vitaly

[Vitaly Repeshko, 2010-03-25 17:08:55]

Thanks for review! Submitted.


-- Vitaly

http://codereview.chromium.org/1327002/diff/1/3
File src/arguments.h (right):

http://codereview.chromium.org/1327002/diff/1/3#newcode75
src/arguments.h:75: // Cursom arguments replicate a small segment of stack that
can be
On 2010/03/25 14:40:53, antonm wrote:
> could you fix it while you are around, please?  Cursom -> Custom.

Done.

http://codereview.chromium.org/1327002/diff/1/3#newcode90
src/arguments.h:90: Object* values_[3];
On 2010/03/25 14:40:53, antonm wrote:
> should we introduce constants to minimize changes?  by no means insisting.

Using ARRAY_SIZE now.

http://codereview.chromium.org/1327002/diff/1/8
File src/stub-cache.cc (right):

http://codereview.chromium.org/1327002/diff/1/8#newcode806
src/stub-cache.cc:806: v8::AccessorInfo info(args.arguments() - 2);
On 2010/03/25 14:40:53, antonm wrote:
> maybe add a special method/constructor?  this 2 looks too magical imho

I don't think it really makes sense to have a method/constructor. Added a named
constant and a few assert-s instead.

http://codereview.chromium.org/1327002/diff/3001/4006
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/1327002/diff/3001/4006#newcode1043
src/ia32/stub-cache-ia32.cc:1043: __ mov(other, esp);
On 2010/03/25 14:40:53, antonm wrote:
> Do we need to push the address?

Yes, please see below. It's internal::Object** from AccessorInfo.

http://codereview.chromium.org/1327002/diff/3001/4006#newcode1054
src/ia32/stub-cache-ia32.cc:1054: __ add(Operand(eax), Immediate(4 *
kPointerSize));
On 2010/03/25 14:40:53, antonm wrote:
> could we use some named constant for this 4?

This is a local thing: it's the depth of the arguments pointer as described in
the comment.

http://codereview.chromium.org/1327002/diff/3001/4006#newcode1058
src/ia32/stub-cache-ia32.cc:1058: ASSERT_EQ(5, ApiGetterEntryStub::kStackSpace);
On 2010/03/25 14:40:53, antonm wrote:
> do you I understand it right, this is not ported to other platforms?  do you
> know if we have corresponding bugs filed?

Right, this is not ported to other platforms and I couldn't find a relevant bug.
Filed http://code.google.com/p/v8/issues/detail?id=663