Direct call accessor getter callbacks (arm implementation).

Direct call accessor getter callbacks (arm implementation).

[Zaheer, 2011-02-17 07:18:28]

Could you please take a look at this CL. Thanks.

[antonm, 2011-02-17 16:13:44]

Thanks a lot for working on this!

http://codereview.chromium.org/6462029/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/6462029/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:1534: typedef v8::Handle<v8::Value>
(*SimulatorRuntimeDirectApiCall)(int32_t arg0);
Do we need word Direct here?  It looks like simulator doesn't have to know it's
anything direct.  But if you feel it conveys important semantic thing, of course
we should keep it.

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

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1242: Register holder_reg =
if you change this, please, change other platforms files as well: we're trying
to keep the things in parallel as much as we can,

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1245: ASSERT(holder_reg.is(receiver) ||
holder_reg.is(scratch1));
do you waht to assert this or rather the fact that !holder_reg.is(scratch{2,3})?

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1249: __ push(receiver);
may I ask you to reformat and comment you code slightly to better communicate
what you're doing here?

I'd suggest:

Comment at ln. 1247 becomes:

// Build AccessorGetter arguments on the stack below exit frame to make GC aware
of them and store pointers to them.
[Empty line]
// Build AccessorInfo on the stack.
__ push(receiver);
__ mov(scratch2, sp);  // scratch2 = &AccessorInfo.
__ push(holder_reg);
....
__ push(sratch3);
[Here comes blank line]
// Push name to the stack.
__ push(name_reg);
__ mov(r0, sp);

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1268: __ str(scratch2, MemOperand(sp, 1 *
kPointerSize));
Please, explain briefly why we need additional wrapping here.

BTW, is this ABI universal across ARM compilers?

http://codereview.chromium.org/6462029/diff/1/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/6462029/diff/1/src/assembler.h#newcode481
src/assembler.h:481: DIRECT_API_CALL,
again question about DIRECT, maybe just drop it, but again no insisting, feel
free to keep it if you like

http://codereview.chromium.org/6462029/diff/1/src/assembler.h#newcode485
src/assembler.h:485: DIRECT_LOAD_CALL
Please, do not use load and use getter or callback instead (or even accessor
getter): we already have this callback/getter distinction and I think we should
introduce another word.

http://codereview.chromium.org/6462029/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6462029/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:7587: v8::V8::LowMemoryNotification();
you can probably directly force GC as v8::internal::Heap is exposed here.  And
yes, we do it :)

http://codereview.chromium.org/6462029/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:7628: CHECK_EQ(v8_str("ggggg"), result);
Thank you very much for the tests!

[SeRya, 2011-02-20 17:28:39]

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

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1260: __ push(name_reg);
__ Push(receiver, scratch3, name_reg) (or at least __ Push(scratch3, name_reg))
apparently should work and save one instruction.

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1268: __ str(scratch2, MemOperand(sp, 1 *
kPointerSize));
Is is an instance of v8::AccessorInfo which consists of a single pointer
internal::Object** args_. Not too hard to conclude from the next comment but may
be worth to mention.

On 2011/02/17 16:13:44, antonm wrote:
> Please, explain briefly why we need additional wrapping here.
> 
> BTW, is this ABI universal across ARM compilers?

[Zaheer, 2011-02-21 10:25:35]

Thanks Anton, Serya for your comments. Please review the latest patch.

http://codereview.chromium.org/6462029/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/6462029/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:1534: typedef v8::Handle<v8::Value>
(*SimulatorRuntimeDirectApiCall)(int32_t arg0);
On 2011/02/17 16:13:44, antonm wrote:
> Do we need word Direct here?  It looks like simulator doesn't have to know
it's
> anything direct.  But if you feel it conveys important semantic thing, of
course
> we should keep it.

I think direct differentiates from the builtin call and i would like to keep it
for now.

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

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1242: Register holder_reg =
On 2011/02/17 16:13:44, antonm wrote:
> if you change this, please, change other platforms files as well: we're trying
> to keep the things in parallel as much as we can,
Do not wish to touch x86, reverted it back to reg.

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1245: ASSERT(holder_reg.is(receiver) ||
holder_reg.is(scratch1));
On 2011/02/17 16:13:44, antonm wrote:
> do you waht to assert this or rather the fact that
!holder_reg.is(scratch{2,3})?
The holder_reg.is(scratch{2,3}) check already happens in checkprototypes.

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1249: __ push(receiver);
On 2011/02/17 16:13:44, antonm wrote:
> may I ask you to reformat and comment you code slightly to better communicate
> what you're doing here?
> 
> I'd suggest:
> 
> Comment at ln. 1247 becomes:
> 
> // Build AccessorGetter arguments on the stack below exit frame to make GC
aware
> of them and store pointers to them.
> [Empty line]
> // Build AccessorInfo on the stack.
> __ push(receiver);
> __ mov(scratch2, sp);  // scratch2 = &AccessorInfo.
> __ push(holder_reg);
> ....
> __ push(sratch3);
> [Here comes blank line]
> // Push name to the stack.
> __ push(name_reg);
> __ mov(r0, sp);

with the merge of pushes, its difficult to separate the blocks. i have modified
the comments.

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1260: __ push(name_reg);
On 2011/02/20 17:28:39, SeRya wrote:
> __ Push(receiver, scratch3, name_reg) (or at least __ Push(scratch3,
name_reg))
> apparently should work and save one instruction.

Done. Merged the last three pushes. cant merge the receiver since it can be same
as holder reg.

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1268: __ str(scratch2, MemOperand(sp, 1 *
kPointerSize));
On 2011/02/17 16:13:44, antonm wrote:
> Please, explain briefly why we need additional wrapping here.
This to create internal::Object** (AccessorInfo only member)
> 
> BTW, is this ABI universal across ARM compilers?
I cant see any ABI/compiler specific code here. May be i miss your point.

http://codereview.chromium.org/6462029/diff/1/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/6462029/diff/1/src/assembler.h#newcode481
src/assembler.h:481: DIRECT_API_CALL,
On 2011/02/17 16:13:44, antonm wrote:
> again question about DIRECT, maybe just drop it, but again no insisting, feel
> free to keep it if you like
keeping it for now to differentiate from builtin.

http://codereview.chromium.org/6462029/diff/1/src/assembler.h#newcode485
src/assembler.h:485: DIRECT_LOAD_CALL
On 2011/02/17 16:13:44, antonm wrote:
> Please, do not use load and use getter or callback instead (or even accessor
> getter): we already have this callback/getter distinction and I think we
should
> introduce another word.

Done.

http://codereview.chromium.org/6462029/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6462029/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:7587: v8::V8::LowMemoryNotification();
On 2011/02/17 16:13:44, antonm wrote:
> you can probably directly force GC as v8::internal::Heap is exposed here.  And
> yes, we do it :)

Done.

[antonm, 2011-02-21 17:45:12]

Almost LGTM, let's do last round and I'll land it

http://codereview.chromium.org/6462029/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/6462029/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:1534: typedef v8::Handle<v8::Value>
(*SimulatorRuntimeDirectApiCall)(int32_t arg0);
On 2011/02/21 10:25:35, Zaheer wrote:
> On 2011/02/17 16:13:44, antonm wrote:
> > Do we need word Direct here?  It looks like simulator doesn't have to know
> it's
> > anything direct.  But if you feel it conveys important semantic thing, of
> course
> > we should keep it.
> 
> I think direct differentiates from the builtin call and i would like to keep
it
> for now.

ok

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

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1245: ASSERT(holder_reg.is(receiver) ||
holder_reg.is(scratch1));
On 2011/02/21 10:25:35, Zaheer wrote:
> On 2011/02/17 16:13:44, antonm wrote:
> > do you waht to assert this or rather the fact that
> !holder_reg.is(scratch{2,3})?
> The holder_reg.is(scratch{2,3}) check already happens in checkprototypes.

So why you bother asserting here?  What kind of information this assert conveys
to the reader?  Sorry, I am probably missing something obvious here.

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1268: __ str(scratch2, MemOperand(sp, 1 *
kPointerSize));
On 2011/02/21 10:25:35, Zaheer wrote:
> On 2011/02/17 16:13:44, antonm wrote:
> > Please, explain briefly why we need additional wrapping here.
> This to create internal::Object** (AccessorInfo only member)
> > 
> > BTW, is this ABI universal across ARM compilers?
> I cant see any ABI/compiler specific code here. May be i miss your point.

I am sorry, I thought that's some additional wrapping.

http://codereview.chromium.org/6462029/diff/8001/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:1247: // Push AccessorInfo arguments and property name
below the
won't it be more correct to phrase it like: build AccessorInfo::args_ list on
the stack and push property name below... ?

http://codereview.chromium.org/6462029/diff/8001/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:1250: __ mov(scratch2, sp);
maybe:  // scratch = AccessorInfo::args_ ?

[Zaheer, 2011-02-23 06:34:07]

hi Anton, Addressed comments. could you take another look.

Thanks.

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

http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1245: ASSERT(holder_reg.is(receiver) ||
holder_reg.is(scratch1));
On 2011/02/21 17:45:12, antonm wrote:
> On 2011/02/21 10:25:35, Zaheer wrote:
> > On 2011/02/17 16:13:44, antonm wrote:
> > > do you waht to assert this or rather the fact that
> > !holder_reg.is(scratch{2,3})?
> > The holder_reg.is(scratch{2,3}) check already happens in checkprototypes.
> 
> So why you bother asserting here?  What kind of information this assert
conveys
> to the reader?  Sorry, I am probably missing something obvious here.
I added this as a sanity check on the return value. Judging from other uses of
CheckPrototypes it may not be required. removed.

http://codereview.chromium.org/6462029/diff/8001/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:1247: // Push AccessorInfo arguments and property name
below the
On 2011/02/21 17:45:12, antonm wrote:
> won't it be more correct to phrase it like: build AccessorInfo::args_ list on
> the stack and push property name below... ?

Done.

http://codereview.chromium.org/6462029/diff/8001/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:1250: __ mov(scratch2, sp);
On 2011/02/21 17:45:12, antonm wrote:
> maybe:  // scratch = AccessorInfo::args_ ?

Done.

[antonm, 2011-02-24 14:43:01]

Thanks a lot, Zaheer.  Landing it.