Mads, tnx a lot for review! I've added more tests to cover all the cases of cacheable lookup (hopefully regular one is tested enough already).
Regarding IsContextual. Thanks a lot for explanations. Is there a way to access this information from StubCompiler? In this case I'd able to compile proper trampoline which won't check contextualness of load. http://codereview.chromium.org/140069/diff/6006/5020 File src/ia32/macro-assembler-ia32.cc (right): http://codereview.chromium.org/140069/diff/6006/5020#newcode476 Line 476: Register MacroAssembler::CheckMaps(JSObject* object, Register object_reg, On 2009/07/03 07:54:23, Mads Ager wrote: > Could you add a comment in the header file explaining that a NULL holder will > generate map checks all the way to the top of the prototype chain? Now obsolete as you spotted that I don't need this functionality anymore---thanks a lot for spotting this! http://codereview.chromium.org/140069/diff/6006/5020#newcode497 Line 497: Object* proto = object->GetPrototype(); On 2009/07/03 07:54:23, Mads Ager wrote: > This code is really part of the loop termination condition. > How about replacing the while with a for: > // Check all maps to the holder or to the top of the prototype chain. > for (Object* proto = object->GetPrototype(); > proto != Heap::null_value() && object != holder; > proto = proto->GetPrototype()) { > // Generate code > } Ditto http://codereview.chromium.org/140069/diff/6006/5024 File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/140069/diff/6006/5024#newcode398 Line 398: struct LoadInterceptorCompiler { On 2009/07/03 07:54:23, Mads Ager wrote: > Make this a class with public and a private part. Put the constructor before > the methods and have 'name' be a private field. > Since these are only supposed to be allocate on the stack, you should use > class LoadInterceptorCompiler BASE_EMBEDDED { > // ... > } Done. http://codereview.chromium.org/140069/diff/6006/5024#newcode407 Line 407: // Prepare tail call to follow On 2009/07/03 07:54:23, Mads Ager wrote: > Add period at the end of comment. Done. http://codereview.chromium.org/140069/diff/6006/5024#newcode443 Line 443: // Cleanup a garbage of prepared second call On 2009/07/03 07:54:23, Mads Ager wrote: > How about just: "Pop lookup result." > Above, you could add a "Push lookup result." comment where you do the three > calls to push. This is not a lookup itself, rather data I needed to save to perform a second call if interceptor fails. I tried to reword it, may you have another look? http://codereview.chromium.org/140069/diff/6006/5024#newcode446 Line 446: __ push(scratch2); On 2009/07/03 07:54:23, Mads Ager wrote: > Do you mean scratch1 here? Do we have a test case that gets here? Oh yes, tnx a lot for spotting this! And probably we don't. Adding it. Actually, you've spotted ways more serious problem (and helped me to fix it :)---I didn't restore ecx when going into miss handle http://codereview.chromium.org/140069/diff/6006/5024#newcode471 Line 471: struct CallInterceptorCompiler { On 2009/07/03 07:54:23, Mads Ager wrote: > Make this a class as well. Done. http://codereview.chromium.org/140069/diff/6006/5025 File src/stub-cache.cc (right): http://codereview.chromium.org/140069/diff/6006/5025#newcode785 Line 785: * but doesn't look down if interceptor returns nothing. On 2009/07/03 07:54:23, Mads Ager wrote: > 'look down' -> 'search the prototype chain'? Done. http://codereview.chromium.org/140069/diff/6006/5026 File src/stub-cache.h (right): http://codereview.chromium.org/140069/diff/6006/5026#newcode308 Line 308: Object* ThrowUndefinedException(Arguments args); On 2009/07/03 07:54:23, Mads Ager wrote: > This one is unused now, isn't it? If so, please remove. Done. http://codereview.chromium.org/140069 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
