Optimizing HandleScope. Also fixed HandleScope destruction when API getter th...

Optimizing HandleScope. Also fixed HandleScope destruction when API getter throws an exception.

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

[SeRya, 2010-10-14 11:59:08]

Generated code simplified a bit by moving exception handling above handle
dereferencing and HandleScope destruction.

[antonm, 2010-10-14 12:03:21]

My main complaint is inlining of of handle scope handling macros.  If you
restore them back, it'd be easier to see how much save is.

BTW, do you have any numbers what is a speedup of this change?

http://codereview.chromium.org/3792003/diff/1/3
File src/api.cc (right):

http://codereview.chromium.org/3792003/diff/1/3#newcode461
src/api.cc:461: : prev_next_(i::HandleScope::current_.next)
nit: I think v8 style mandates comma at the end of initialization

http://codereview.chromium.org/3792003/diff/1/3#newcode477
src/api.cc:477: i::HandleScope::current_.level--;
ASSERT(current_.level >= 0)?

http://codereview.chromium.org/3792003/diff/1/4
File src/api.h (right):

http://codereview.chromium.org/3792003/diff/1/4#newcode471
src/api.h:471: internal::Object** block_limit = &block_start[kHandleBlockSize];
why not block_start + kHandleBlockSize?

http://codereview.chromium.org/3792003/diff/1/9
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/1/9#newcode3115
src/ia32/code-stubs-ia32.cc:3115:
ExternalReference::handle_scope_next_address();
nit: I think you need 4 spaces alignment here

http://codereview.chromium.org/3792003/diff/1/9#newcode3124
src/ia32/code-stubs-ia32.cc:3124: __
inc(Operand::StaticVariable(level_address));
inc/dec should be avoided as per Intel Optimization Guide

http://codereview.chromium.org/3792003/diff/1/9#newcode3131
src/ia32/code-stubs-ia32.cc:3131: __
dec(Operand::StaticVariable(level_address));
Please, add a debug check that level is >= 0

http://codereview.chromium.org/3792003/diff/1/9#newcode3158
src/ia32/code-stubs-ia32.cc:3158: // Finish restoring previous handle scope.
I don't think we should have this complicated control flow: if exception has
been thrown, we're already on a slow path and two move instructions won't save
us.

http://codereview.chromium.org/3792003/diff/1/11
File src/ia32/macro-assembler-ia32.h (left):

http://codereview.chromium.org/3792003/diff/1/11#oldcode483
src/ia32/macro-assembler-ia32.h:483: void PushHandleScope(Register scratch);
why those macros are removed?  I'd prefer if new logic would be implemented in
macros (see also comments about implementation), .e.g, we could reuse it to
invoke other API stuff from generated code.

http://codereview.chromium.org/3792003/diff/1/16
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/3792003/diff/1/16#newcode2544
src/x64/code-stubs-x64.cc:2544: __ incl(Operand(base_reg, kLevelOffset));
ditto for incl/decl

http://codereview.chromium.org/3792003/diff/1/16#newcode2550
src/x64/code-stubs-x64.cc:2550: __ movq(rsi, rdx);
apparently rsi a.k.a. base_reg is clobbered here, so why you don't need to
restore it below (see ln. 2563)?

http://codereview.chromium.org/3792003/diff/1/16#newcode2562
src/x64/code-stubs-x64.cc:2562: // rsi is only callee save on Win64. On other
platforms it must be restored.
nit: save -> save[d]

[SeRya, 2010-10-15 13:31:33]

http://codereview.chromium.org/3792003/diff/1/3
File src/api.cc (right):

http://codereview.chromium.org/3792003/diff/1/3#newcode461
src/api.cc:461: : prev_next_(i::HandleScope::current_.next)
On 2010/10/14 12:03:21, antonm wrote:
> nit: I think v8 style mandates comma at the end of initialization

Done.

http://codereview.chromium.org/3792003/diff/1/3#newcode477
src/api.cc:477: i::HandleScope::current_.level--;
On 2010/10/14 12:03:21, antonm wrote:
> ASSERT(current_.level >= 0)?

Done.

http://codereview.chromium.org/3792003/diff/1/4
File src/api.h (right):

http://codereview.chromium.org/3792003/diff/1/4#newcode471
src/api.h:471: internal::Object** block_limit = &block_start[kHandleBlockSize];
On 2010/10/14 12:03:21, antonm wrote:
> why not block_start + kHandleBlockSize?

Done.

http://codereview.chromium.org/3792003/diff/1/9
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/1/9#newcode3115
src/ia32/code-stubs-ia32.cc:3115:
ExternalReference::handle_scope_next_address();
On 2010/10/14 12:03:21, antonm wrote:
> nit: I think you need 4 spaces alignment here

Done.

http://codereview.chromium.org/3792003/diff/1/9#newcode3124
src/ia32/code-stubs-ia32.cc:3124: __
inc(Operand::StaticVariable(level_address));
On 2010/10/14 12:03:21, antonm wrote:
> inc/dec should be avoided as per Intel Optimization Guide

Changed to add/sub

http://codereview.chromium.org/3792003/diff/1/9#newcode3131
src/ia32/code-stubs-ia32.cc:3131: __
dec(Operand::StaticVariable(level_address));
On 2010/10/14 12:03:21, antonm wrote:
> Please, add a debug check that level is >= 0

Done.

http://codereview.chromium.org/3792003/diff/1/9#newcode3158
src/ia32/code-stubs-ia32.cc:3158: // Finish restoring previous handle scope.
On 2010/10/14 12:03:21, antonm wrote:
> I don't think we should have this complicated control flow: if exception has
> been thrown, we're already on a slow path and two move instructions won't save
> us.

Done.

http://codereview.chromium.org/3792003/diff/1/11
File src/ia32/macro-assembler-ia32.h (left):

http://codereview.chromium.org/3792003/diff/1/11#oldcode483
src/ia32/macro-assembler-ia32.h:483: void PushHandleScope(Register scratch);
On 2010/10/14 12:03:21, antonm wrote:
> why those macros are removed?  I'd prefer if new logic would be implemented in
> macros (see also comments about implementation), .e.g, we could reuse it to
> invoke other API stuff from generated code.

As we discussed I introduced TailCallApiFunction macro.

http://codereview.chromium.org/3792003/diff/1/16
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/3792003/diff/1/16#newcode2544
src/x64/code-stubs-x64.cc:2544: __ incl(Operand(base_reg, kLevelOffset));
On 2010/10/14 12:03:21, antonm wrote:
> ditto for incl/decl

Done.

http://codereview.chromium.org/3792003/diff/1/16#newcode2550
src/x64/code-stubs-x64.cc:2550: __ movq(rsi, rdx);
On 2010/10/14 12:03:21, antonm wrote:
> apparently rsi a.k.a. base_reg is clobbered here, so why you don't need to
> restore it below (see ln. 2563)?

It's clobbered unless _WIN64. In this case it's restored there. So it's OK.

http://codereview.chromium.org/3792003/diff/1/16#newcode2562
src/x64/code-stubs-x64.cc:2562: // rsi is only callee save on Win64. On other
platforms it must be restored.
On 2010/10/14 12:03:21, antonm wrote:
> nit: save -> save[d]

They named in such a way in docs ("Callee-save registers are registers that you
have to
save before using them and restore after using them (also called non-volatile
registers)"). So let's be consistent.

[antonm, 2010-10-18 12:45:29]

LGTM.

Q: do you have most fresh numbers for performance impact?

As a thought: if even a single move counts, couldn't we squeeze next and limit
into a single pointer?  For example, if we properly align blocks and allocate in
2^n blocks, next could be easily obtained from next like something like next &
0xffff0000 + 0x10000.

http://codereview.chromium.org/3792003/diff/43019/13031
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/43019/13031#newcode1088
src/ia32/macro-assembler-ia32.cc:1088: void
MacroAssembler::TailCallApiFunction(ApiFunction* function,
completely optional: if we ever consider different strategy for handling of
empty returned results (think of interceptors), it might be handy to have a hook
for it already---just pass on empty result and come back labels.

http://codereview.chromium.org/3792003/diff/43019/13032
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/3792003/diff/43019/13032#newcode484
src/ia32/macro-assembler-ia32.h:484: // returned value from handle and
propogates exceptions.
maybe add some words about what this macro expects in regard of stack, parameter
passing, etc.

http://codereview.chromium.org/3792003/diff/43019/13038
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/3792003/diff/43019/13038#newcode518
src/x64/macro-assembler-x64.cc:518: // rsi is only callee-save on Win64. On
other platforms it must be restored.
as was discussed offline, that's not the best explanation why it is allowed not
to restore base_reg on Win64

http://codereview.chromium.org/3792003/diff/43019/13039
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/3792003/diff/43019/13039#newcode824
src/x64/macro-assembler-x64.h:824: void TailCallApiFunction(ApiFunction*
function, int stack_space);
Ditto for better documentation

[SeRya, 2010-10-20 11:10:27]

I measured small boost (about 2.5% on DOM queries that is bigger than variations
~0.45% though). It's smaller than I previously measured on Lin64 before fixing
the bug with exceptions, but not this CL fixed the bug what is good.

http://codereview.chromium.org/3792003/diff/43019/13032
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/3792003/diff/43019/13032#newcode484
src/ia32/macro-assembler-ia32.h:484: // returned value from handle and
propogates exceptions.
On 2010/10/18 12:45:29, antonm wrote:
> maybe add some words about what this macro expects in regard of stack,
parameter
> passing, etc.

Done.

http://codereview.chromium.org/3792003/diff/43019/13038
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/3792003/diff/43019/13038#newcode518
src/x64/macro-assembler-x64.cc:518: // rsi is only callee-save on Win64. On
other platforms it must be restored.
On 2010/10/18 12:45:29, antonm wrote:
> as was discussed offline, that's not the best explanation why it is allowed
not
> to restore base_reg on Win64

The comment rewritten.

http://codereview.chromium.org/3792003/diff/43019/13039
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/3792003/diff/43019/13039#newcode824
src/x64/macro-assembler-x64.h:824: void TailCallApiFunction(ApiFunction*
function, int stack_space);
On 2010/10/18 12:45:29, antonm wrote:
> Ditto for better documentation

Done.

[antonm, 2010-10-20 11:17:13]

http://codereview.chromium.org/3792003/diff/78001/79011
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/78001/79011#newcode1101
src/ia32/macro-assembler-ia32.cc:1101: mov(Operand(esp, 0 * kPointerSize), ebx);
 // name.
I don't think that belongs here---it should be on caller side.  I think call api
function should only call api function and analyse the result.

And I don't quite understand why it's named TailCall

http://codereview.chromium.org/3792003/diff/78001/79012
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/3792003/diff/78001/79012#newcode486
src/ia32/macro-assembler-ia32.h:486: //  eax - pointer to accessor_info.
Sorry, I missed that in review, but that's not good imho: TailCallApiFunction
should be generic, not to call accessors (otherwise it should be named so), see
comment in macro-assembler-ia32.cc

[SeRya, 2010-10-20 18:37:50]

TailCallApiFunction generalized, PrepareCallApiFunction added.

[antonm, 2010-10-21 11:52:37]

Thanks a lot for refactoring API function invocation.

http://codereview.chromium.org/3792003/diff/123001/80016
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/123001/80016#newcode3064
src/ia32/code-stubs-ia32.cc:3064: __ mov(Operand(esp,
(MacroAssembler::kFistApiParameter + 0) * kPointerSize),
as you introduce kFirstApiParameter constant anyway, maybe add
MoveApiParemeter(int index, Operand) macro instead?

http://codereview.chromium.org/3792003/diff/123001/80017
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1101
src/ia32/macro-assembler-ia32.cc:1101: Label empty_handle;
could we move labels closer to the place they are used?

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1119
src/ia32/macro-assembler-ia32.cc:1119: lea(eax, Operand(esp, (argc + 1) *
kPointerSize, RelocInfo::NONE));
I don't think you need to explicitly give reloc info here

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1119
src/ia32/macro-assembler-ia32.cc:1119: lea(eax, Operand(esp, (argc + 1) *
kPointerSize, RelocInfo::NONE));
Please, add a comment that you load a pointer to output handle here

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1121
src/ia32/macro-assembler-ia32.cc:1121: mov(Operand(esp, (argc + 1) *
kPointerSize), Immediate(0));  // out cell.
couldn't you use mov(Operand(eax, 0), Immediate(0));  instead?

http://codereview.chromium.org/3792003/diff/123001/80018
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/3792003/diff/123001/80018#newcode489
src/ia32/macro-assembler-ia32.h:489: // Prepares stack to put arguments
overthere (aligns and so on). Reserves
overthere -> over there, or better there or just omit it: Prepares stack for
arguments.

http://codereview.chromium.org/3792003/diff/123001/80018#newcode490
src/ia32/macro-assembler-ia32.h:490: // space for return value if needs (assumes
the return value is a handle).
needs -> needed

http://codereview.chromium.org/3792003/diff/123001/80018#newcode491
src/ia32/macro-assembler-ia32.h:491: // Uses calle-saved esi to restore stack
state after call. Arguments must be
calle<e>

http://codereview.chromium.org/3792003/diff/123001/80018#newcode496
src/ia32/macro-assembler-ia32.h:496: // Tail call an API function (jump).
Allocates HandleScope, extracts
What I miss: I see no tail call to API function?

http://codereview.chromium.org/3792003/diff/123001/80018#newcode497
src/ia32/macro-assembler-ia32.h:497: // returned value from handle and
propogates exceptions.
prop<a>gates

http://codereview.chromium.org/3792003/diff/123001/80024
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/3792003/diff/123001/80024#newcode500
src/x64/macro-assembler-x64.cc:500: Register base_reg = kSmiConstantRegister; 
// callee-save on Win64.
it seems to be callee save on both Win64 and Linux, no?

http://codereview.chromium.org/3792003/diff/123001/80025
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/3792003/diff/123001/80025#newcode822
src/x64/macro-assembler-x64.h:822: // Prepares stack to put arguments overthere
(aligns and so on).
Same as for ia32

http://codereview.chromium.org/3792003/diff/123001/80025#newcode824
src/x64/macro-assembler-x64.h:824: // stored in esp[0], esp[4] etc.
this appears to be an incorrect copy paste from former ia32 doc

[SeRya, 2010-10-21 13:04:06]

http://codereview.chromium.org/3792003/diff/123001/80016
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/123001/80016#newcode3064
src/ia32/code-stubs-ia32.cc:3064: __ mov(Operand(esp,
(MacroAssembler::kFistApiParameter + 0) * kPointerSize),
On 2010/10/21 11:52:38, antonm wrote:
> as you introduce kFirstApiParameter constant anyway, maybe add
> MoveApiParemeter(int index, Operand) macro instead?

Added Operand ApiParameterOperand(int index) instead.

http://codereview.chromium.org/3792003/diff/123001/80017
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1101
src/ia32/macro-assembler-ia32.cc:1101: Label empty_handle;
On 2010/10/21 11:52:38, antonm wrote:
> could we move labels closer to the place they are used?

Done.

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1119
src/ia32/macro-assembler-ia32.cc:1119: lea(eax, Operand(esp, (argc + 1) *
kPointerSize, RelocInfo::NONE));
On 2010/10/21 11:52:38, antonm wrote:
> I don't think you need to explicitly give reloc info here

Done.

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1119
src/ia32/macro-assembler-ia32.cc:1119: lea(eax, Operand(esp, (argc + 1) *
kPointerSize, RelocInfo::NONE));
On 2010/10/21 11:52:38, antonm wrote:
> Please, add a comment that you load a pointer to output handle here

Done.

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1121
src/ia32/macro-assembler-ia32.cc:1121: mov(Operand(esp, (argc + 1) *
kPointerSize), Immediate(0));  // out cell.
On 2010/10/21 11:52:38, antonm wrote:
> couldn't you use mov(Operand(eax, 0), Immediate(0));  instead?

Sure we could. It would save one byte of code but wouldn't it introduce data
dependence?

http://codereview.chromium.org/3792003/diff/123001/80018
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/3792003/diff/123001/80018#newcode489
src/ia32/macro-assembler-ia32.h:489: // Prepares stack to put arguments
overthere (aligns and so on). Reserves
On 2010/10/21 11:52:38, antonm wrote:
> overthere -> over there, or better there or just omit it: Prepares stack for
> arguments.

Done.

http://codereview.chromium.org/3792003/diff/123001/80018#newcode490
src/ia32/macro-assembler-ia32.h:490: // space for return value if needs (assumes
the return value is a handle).
On 2010/10/21 11:52:38, antonm wrote:
> needs -> needed

Done.

http://codereview.chromium.org/3792003/diff/123001/80018#newcode491
src/ia32/macro-assembler-ia32.h:491: // Uses calle-saved esi to restore stack
state after call. Arguments must be
On 2010/10/21 11:52:38, antonm wrote:
> calle<e>

Done.

http://codereview.chromium.org/3792003/diff/123001/80018#newcode496
src/ia32/macro-assembler-ia32.h:496: // Tail call an API function (jump).
Allocates HandleScope, extracts
On 2010/10/21 11:52:38, antonm wrote:
> What I miss: I see no tail call to API function?

It actually does call + ret that logical equivalent to jump.

http://codereview.chromium.org/3792003/diff/123001/80018#newcode497
src/ia32/macro-assembler-ia32.h:497: // returned value from handle and
propogates exceptions.
On 2010/10/21 11:52:38, antonm wrote:
> prop<a>gates

Done.

http://codereview.chromium.org/3792003/diff/123001/80024
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/3792003/diff/123001/80024#newcode500
src/x64/macro-assembler-x64.cc:500: Register base_reg = kSmiConstantRegister; 
// callee-save on Win64.
On 2010/10/21 11:52:38, antonm wrote:
> it seems to be callee save on both Win64 and Linux, no?

Done.

http://codereview.chromium.org/3792003/diff/123001/80025
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/3792003/diff/123001/80025#newcode822
src/x64/macro-assembler-x64.h:822: // Prepares stack to put arguments overthere
(aligns and so on).
On 2010/10/21 11:52:38, antonm wrote:
> Same as for ia32

Done.

http://codereview.chromium.org/3792003/diff/123001/80025#newcode824
src/x64/macro-assembler-x64.h:824: // stored in esp[0], esp[4] etc.
On 2010/10/21 11:52:38, antonm wrote:
> this appears to be an incorrect copy paste from former ia32 doc

Done.

[SeRya, 2010-10-21 13:38:16]

TailCallApiFunction renamed to CallApiFunctionAndReturn

[antonm, 2010-10-21 13:53:45]

LGTM

http://codereview.chromium.org/3792003/diff/123001/80017
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/3792003/diff/123001/80017#newcode1121
src/ia32/macro-assembler-ia32.cc:1121: mov(Operand(esp, (argc + 1) *
kPointerSize), Immediate(0));  // out cell.
On 2010/10/21 13:04:06, SeRya wrote:
> On 2010/10/21 11:52:38, antonm wrote:
> > couldn't you use mov(Operand(eax, 0), Immediate(0));  instead?
> 
> Sure we could. It would save one byte of code but wouldn't it introduce data
> dependence?

Up to you.  Given you have eax dependency above and doing tons of additional
stuff below, I'd be surprised if it matters.