Overall the change looks sound, but I think you should change it so that you only have one runtime call per possibly direct eval. Comments:
http://codereview.chromium.org/12673/diff/1/9 File src/ast.h (right): http://codereview.chromium.org/12673/diff/1/9#newcode868 Line 868: enum EvalType { Do you really want this in all Calls? How about subclassing Call (something like CallEval) and putting this behaviour in there? http://codereview.chromium.org/12673/diff/1/15 File src/codegen-arm.cc (right): http://codereview.chromium.org/12673/diff/1/15#newcode2323 Line 2323: Label after_call; Move after_call declaration and binding to inside the then part of the if (node->eval_type...). http://codereview.chromium.org/12673/diff/1/15#newcode2329 Line 2329: __ CallRuntime(Runtime::kInDirectEval, 1); How about letting the runtime call return a function that you just need to call? In case of a direct eval, you would get some new dynamically generated function back, but in the other cases you would just get the global.eval function back. This avoids two out of three runtime calls and also the string type testing. The only slight complication is dealing with the arguments and the receiver on the stack, but that should be doable. http://codereview.chromium.org/12673/diff/1/15#newcode2463 Line 2463: __ bind(&after_call); Move inside if then-part. http://codereview.chromium.org/12673/diff/1/20 File src/codegen-ia32.cc (right): http://codereview.chromium.org/12673/diff/1/20#newcode2693 Line 2693: Label after_call; See my comments for the ARM code. http://codereview.chromium.org/12673/diff/1/17 File src/macro-assembler-arm.cc (right): http://codereview.chromium.org/12673/diff/1/17#newcode746 Line 746: void MacroAssembler::GetInstanceType(Register holder) { Wouldn't it be better to pass in a Label we could jump to for non heap objects? It seems like an overkill to materialize INVALID_TYPE and SMI_TYPE ever. http://codereview.chromium.org/12673/diff/1/17#newcode750 Line 750: tst(holder, Operand(kFailureTagMask)); You shouldn't expect to see failure tagged pointers ever. I think this code should be removed or at least hidden behind the debug-code flag. It shouldn't happen. http://codereview.chromium.org/12673/diff/1/7 File src/macro-assembler-ia32.cc (right): http://codereview.chromium.org/12673/diff/1/7#newcode702 Line 702: void MacroAssembler::GetInstanceType(Register holder) { See comments for ARM. http://codereview.chromium.org/12673/diff/1/19 File src/parser.cc (right): http://codereview.chromium.org/12673/diff/1/19#newcode659 Line 659: Intentional? http://codereview.chromium.org/12673/diff/1/19#newcode2643 Line 2643: for (Scope* scope = top_scope_; Move this code to the Scope implementation; maybe even rename the current Scope::Lookup to Scope::LookupLocal and call your new code Scope::Lookup. http://codereview.chromium.org/12673 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
