Avoiding repacking payload for v8::Arguments and v8::AccessorInfo (arm).

Avoiding repacking payload for v8::Arguments and v8::AccessorInfo (arm)


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

[antonm, 2010-11-17 14:10:29]

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

http://codereview.chromium.org/5107002/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1084: __ push(receiver);  // Receiver
nit: probably change R -> r as you remove a dot.

http://codereview.chromium.org/5107002/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1085: __ mov(ip,
Operand(Handle<AccessorInfo>(callback)));  // callback data
why such a change, __ Push(reg, ip, name_reg) apparently should have done the
trick, what am I missing

http://codereview.chromium.org/5107002/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1211: __ ldr(scratch3,
why scratch3?  it looks like the diff might be made easier to read by something
like:

__ Push(receiver, holder_reg);
__ ldr(....)
__ Push(.......)

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc
File src/builtins.cc (left):

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc#oldcode1102
src/builtins.cc:1102: custom.end(),
do we use CustomArguments?  ::end()?

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc#newcode1091
src/builtins.cc:1091: Handle<JSFunction> function =
args.at<JSFunction>(args_length);
maybe move this check after new_args construction and use new_args.Callee() and
new_args.This()?

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

http://codereview.chromium.org/5107002/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:7019: CHECK_EQ(args.Data(), v8_str("method_data"));
nit: indent

[SeRya, 2010-11-17 15:36:29]

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

http://codereview.chromium.org/5107002/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1084: __ push(receiver);  // Receiver
On 2010/11/17 14:10:32, antonm wrote:
> nit: probably change R -> r as you remove a dot.

Reverted change in this line.

http://codereview.chromium.org/5107002/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1085: __ mov(ip,
Operand(Handle<AccessorInfo>(callback)));  // callback data
On 2010/11/17 14:10:32, antonm wrote:
> why such a change, __ Push(reg, ip, name_reg) apparently should have done the
> trick, what am I missing

It would work, but generates more instructions. |Push| preferes arguments with
decreasing codes. ip=r13, scratch3=r4, name_reg=r0|r2. So here are 2 decreasing
subsequences. Actually I was wrong (thought it needs increasing codes) and now
fixing.

http://codereview.chromium.org/5107002/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:1211: __ ldr(scratch3,
On 2010/11/17 14:10:32, antonm wrote:
> why scratch3?  it looks like the diff might be made easier to read by
something
> like:
> 
> __ Push(receiver, holder_reg);
> __ ldr(....)
> __ Push(.......)

Done.

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc
File src/builtins.cc (left):

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc#oldcode1102
src/builtins.cc:1102: custom.end(),
On 2010/11/17 14:10:32, antonm wrote:
> do we use CustomArguments?  ::end()?

We do.

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc#newcode1091
src/builtins.cc:1091: Handle<JSFunction> function =
args.at<JSFunction>(args_length);
On 2010/11/17 14:10:32, antonm wrote:
> maybe move this check after new_args construction and use new_args.Callee()
and
> new_args.This()?

new_args.Callee() is v8::Handle. Do you think it worth to convert it back and
forth?

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

http://codereview.chromium.org/5107002/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:7019: CHECK_EQ(args.Data(), v8_str("method_data"));
On 2010/11/17 14:10:32, antonm wrote:
> nit: indent

It was for debugging. Reverted.

[antonm, 2010-11-18 11:36:08]

LGTM with comment addressed

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/5107002/diff/1/src/builtins.cc#newcode1091
src/builtins.cc:1091: Handle<JSFunction> function =
args.at<JSFunction>(args_length);
On 2010/11/17 15:36:29, SeRya wrote:
> On 2010/11/17 14:10:32, antonm wrote:
> > maybe move this check after new_args construction and use new_args.Callee()
> and
> > new_args.This()?
> 
> new_args.Callee() is v8::Handle. Do you think it worth to convert it back and
> forth?

I don't think we care in debug mode.  And those handles should be ephemeral if
memory serves: just a pointer inside an array of which GC is aware

[SeRya, 2010-11-18 15:36:32]

I found a bug in prev CL, so please review again.

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

http://codereview.chromium.org/5107002/diff/10001/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:1212: FieldMemOperand(holder_reg,
AccessorInfo::kDataOffset));
Using holder_reg with AccessorInfo::kDataOffset doesn't make sence, but
previously this value was ignored (callback->data() was used instead). When I
begun to use this field the bug was not discovered because tests what cover this
code didn't check Data() field. So I added such a check into test-api.cc.

http://codereview.chromium.org/5107002/diff/1008/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/5107002/diff/1008/src/builtins.cc#newcode1099
src/builtins.cc:1099: #endif
On 2010/11/18 11:36:08, antonm wrote:
> On 2010/11/17 15:36:29, SeRya wrote:
> > On 2010/11/17 14:10:32, antonm wrote:
> > > maybe move this check after new_args construction and use
new_args.Callee()
> > and
> > > new_args.This()?
> > 
> > new_args.Callee() is v8::Handle. Do you think it worth to convert it back
and
> > forth?
> 
> I don't think we care in debug mode.  And those handles should be ephemeral if
> memory serves: just a pointer inside an array of which GC is aware

Done. Actually it was a bug here (|function| was at a wrong place). I fixed it
and added type check into apiutils.h. Thank you for the comment.

[antonm, 2010-11-18 16:06:37]

LGTM.

And before you submit it, please, fix the title:

Avoi[g]ing repacking payload for v[9]::Arguments and v8::AccessorInfo (arm)

should be

Avoiding repacking payload for v8::Arguments and v8::AccessorInfo (arm)