Mads, I did a comparison of interceptor and hand-coded forwarding, the differences are significant.
On V8 benchsuite, it is 2542 vs. 2982 (bigger is better). I think interceptor kills inline cache. There is one place might help inline cache performance: In codegen-ia32: VisitCall, when generating code for 'foo(...)', currently it pushes the global receiver (outer window object) on the stack as the receiver. It is used for both method lookup and as 'this' for invocation. I'd like to use inner global for lookup, but use outer global as receiver passing to the function, this can speed up the global function invocation a bit. But it is hard to do it now. http://codereview.chromium.org/7366/diff/1/8 File src/api.cc (right): http://codereview.chromium.org/7366/diff/1/8#newcode2298 Line 2298: if (IsDeadCheck("v8::Context::RestoreSecurityToken()")) return; On 2008/10/17 11:11:18, Mads Ager wrote: > RestoreSecurityToken -> UseDefaultSecurityToken. Done. http://codereview.chromium.org/7366/diff/1/8#newcode2358 Line 2358: i::Bootstrapper::DetachGlobal(context); Because the logic of DetachGlobal corresponds to what's in CreateEnvironment, that's why I think bootstreapper.cc is the place to put it there. I agree that by name, it's better be a function of Context itself. That's what is in api. But bootstrapper.cc is a better place to put the implementation, I think. On 2008/10/17 11:11:18, Mads Ager wrote: > Why is DetachGlobal on the bootstrapper? It does not appear to be used during > bootstrapping. http://codereview.chromium.org/7366/diff/1/18 File src/bootstrapper.cc (right): http://codereview.chromium.org/7366/diff/1/18#newcode351 Line 351: void Bootstrapper::DetachGlobal(Handle<Context> env) { On 2008/10/17 11:11:18, Mads Ager wrote: > Move this and it's helper to handles.[h,cpp]? See my explanation in api.cc. http://codereview.chromium.org/7366/diff/1/13 File src/bootstrapper.h (right): http://codereview.chromium.org/7366/diff/1/13#newcode50 Line 50: static void DetachGlobal(Handle<Context> env); On 2008/10/17 11:11:18, Mads Ager wrote: > Move this to handles.[h,cpp]? See my explanation in api.cc. http://codereview.chromium.org/7366/diff/1/31 File src/factory.h (right): http://codereview.chromium.org/7366/diff/1/31#newcode275 Line 275: ApiInstanceType type = JavaScriptObject); On 2008/10/17 11:11:18, Mads Ager wrote: > Indentation: align with other argument. Done. http://codereview.chromium.org/7366/diff/1/7 File src/macro-assembler-ia32.cc (left): http://codereview.chromium.org/7366/diff/1/7#oldcode486 Line 486: // Restore scratch register to be the map of the object. We On 2008/10/17 11:11:18, Mads Ager wrote: > This comment makes sense to me. Leave it in? Done. http://codereview.chromium.org/7366/diff/1/7 File src/macro-assembler-ia32.cc (right): http://codereview.chromium.org/7366/diff/1/7#newcode485 Line 485: // Pass in the prototype JSGlobalObject Removed. On 2008/10/17 11:11:18, Mads Ager wrote: > Out-dated comment? http://codereview.chromium.org/7366/diff/1/7#newcode582 Line 582: // Read the first work and compare to global_context_map(), On 2008/10/17 11:11:18, Mads Ager wrote: > work -> word Done. http://codereview.chromium.org/7366/diff/1/16 File src/objects.cc (right): http://codereview.chromium.org/7366/diff/1/16#newcode1372 Line 1372: return JSObject::cast(proto)->LocalLookup(name, result); Good catch, it should be LocalLookupRealNamedProperty; On 2008/10/17 11:11:18, Mads Ager wrote: > Should this be LocalLookupRealNamedProperty? > > Also, can we avoid all this special casing for the ApiGlobalObject by installing > interceptors on the ApiGlobalObject that forward all access to the prototype? > It would be great to avoid all these extra checks. I agree with you, I wasn't certain if the current way is the right thing to do. On the other hand, does interceptor prevents inline-caching? http://codereview.chromium.org/7366/diff/1/28 File src/objects.h (right): http://codereview.chromium.org/7366/diff/1/28#newcode2748 Line 2748: // An ApiGlobalObject can be reinitialized by keeping its identity. On 2008/10/17 11:11:18, Mads Ager wrote: > by keeping -> which will preserve? Done. http://codereview.chromium.org/7366/diff/1/26 File src/runtime.cc (right): http://codereview.chromium.org/7366/diff/1/26#newcode3710 Line 3710: static Object* Runtime_GlobalThis(Arguments args) { On 2008/10/17 11:11:18, Mads Ager wrote: > Should this be called GlobalReceiver for consistency? Done. http://codereview.chromium.org/7366/diff/1/24 File src/runtime.js (right): http://codereview.chromium.org/7366/diff/1/24#newcode77 Line 77: } else if (x == null || IS_UNDEFINED(x)) { This is a drive-by optimization for comparing 'x == undefined'. The current implementation always falls into the slowest case for such comparison. I can take this out if you feels it should not be part of this CB. On 2008/10/17 11:11:18, Mads Ager wrote: > I'm not sure I understand these changes. Did the old code cause problems > because of undetectable objects? http://codereview.chromium.org/7366/diff/1/22 File src/top.h (right): http://codereview.chromium.org/7366/diff/1/22#newcode233 Line 233: // a builtin object, or a js globla object. On 2008/10/17 11:11:18, Mads Ager wrote: > globla -> global Done. http://codereview.chromium.org/7366 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
