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] http://codereview.chromium.org/3792003/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
