Kasper, tnx a lot for review! Hopefully addressed everything.
I reworked a bit case with callbacks and invalidation as NULL setter make property readonly and __proto__ games I used to play apparently invalidate maps and thus don't allow to prove necessity of CheckPrototypes. When you're fine with this CL, I'd cook up a simplified version which only compiles const functions (if it'll give us another speed up). yours, anton. http://codereview.chromium.org/140069/diff/6055/6057 File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/140069/diff/6055/6057#newcode365 Line 365: if (lookup->type() == FIELD) { On 2009/07/15 07:01:35, Kasper Lund wrote: > I think you should a comment here that explains in words what kind of > intercepted loads you can optimize. Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode371 Line 371: optimize = callback->getter() != 0; On 2009/07/15 07:01:35, Kasper Lund wrote: > 0 -> NULL? Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode380 Line 380: __ EnterInternalFrame(); On 2009/07/15 07:01:35, Kasper Lund wrote: > Maybe explain why you need a frame here? Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode409 Line 409: holder = masm->CheckMaps(holder_obj, holder, lookup->holder(), On 2009/07/15 07:01:35, Kasper Lund wrote: > CheckPrototypes. Done with a test which failed before. http://codereview.chromium.org/140069/diff/6055/6057#newcode418 Line 418: ASSERT(callback); On 2009/07/15 07:01:35, Kasper Lund wrote: > callback != NULL Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode419 Line 419: ASSERT(callback->getter()); On 2009/07/15 07:01:35, Kasper Lund wrote: > callback->getter() != NULL Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode426 Line 426: holder = masm->CheckMaps(holder_obj, holder, lookup->holder(), On 2009/07/15 07:01:35, Kasper Lund wrote: > CheckPrototypes. Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode463 Line 463: explicit LoadInterceptorCompiler(Register name) : name(name) {} On 2009/07/15 07:01:35, Kasper Lund wrote: > Constructor should go before the methods. Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode466 Line 466: Register name; On 2009/07/15 07:01:35, Kasper Lund wrote: > name_? Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode485 Line 485: if (function->is_compiled() && !holder_obj->IsJSArray()) { On 2009/07/15 07:01:35, Kasper Lund wrote: > I think this deserves a comment. Why is it a problem if holder_obj is a JS > array? Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode512 Line 512: masm->CheckMaps(holder_obj, receiver, lookup->holder(), On 2009/07/15 07:01:35, Kasper Lund wrote: > Please rewrite using CheckPrototypes. Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode557 Line 557: explicit CallInterceptorCompiler(const ParameterCount& arguments) On 2009/07/15 07:01:35, Kasper Lund wrote: > Constructor before methods in definition. Done. http://codereview.chromium.org/140069/diff/6055/6057#newcode561 Line 561: const ParameterCount& arguments; On 2009/07/15 07:01:35, Kasper Lund wrote: > Any good reason not to call these arguments_ and argc_? Nope, historical artifacts. Done. http://codereview.chromium.org/140069/diff/6055/6056 File test/cctest/test-api.cc (right): http://codereview.chromium.org/140069/diff/6055/6056#newcode5028 Line 5028: // a stub, but interceptor produced value on its own On 2009/07/15 07:01:35, Kasper Lund wrote: > Terminate comment with . Done. http://codereview.chromium.org/140069/diff/6055/6056#newcode5048 Line 5048: // a stub, but it got invalidated later on On 2009/07/15 07:01:35, Kasper Lund wrote: > on -> on. Done. http://codereview.chromium.org/140069/diff/6055/6056#newcode5140 Line 5140: // a stub, but interceptor produced value on its own On 2009/07/15 07:01:35, Kasper Lund wrote: > own -> own. Done. http://codereview.chromium.org/140069/diff/6055/6056#newcode5168 Line 5168: // a stub, but it got invalidated later on On 2009/07/15 07:01:35, Kasper Lund wrote: > Ditto. Done. http://codereview.chromium.org/140069/diff/6055/6056#newcode5286 Line 5286: // a value, we can fetch regular value On 2009/07/15 07:01:35, Kasper Lund wrote: > Ditto. Done. http://codereview.chromium.org/140069 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
