A few nits: - The description has typos and generally doesn't make a lot of sense. - You sent the review email to the public list, but you are on the internal google-only Rietveld instance. You should be using the public one (and not leaking internal Google URLs to public lists).
Thanks -- dean On 2009/05/28 06:18:06, Kasper Lund wrote: > I really like this. Comments: > http://chromereview.prom.corp.google.com/1033044/diff/2001/1004 > File src/ia32/stub-cache-ia32.cc (right): http://chromereview.prom.corp.google.com/1033044/diff/2001/1004#newcode357 > Line 357: __ push(Immediate(fast_index)); // fast_index > Critical: Push as smi to avoid GC issues. http://chromereview.prom.corp.google.com/1033044/diff/2001/1004#newcode689 > Line 689: __ push(Immediate(-1)); // fast_index > Push as smi. Use named constant. http://chromereview.prom.corp.google.com/1033044/diff/2001/1004#newcode995 > Line 995: fast_index = -1; // Need to relookup :( > Named constant. http://chromereview.prom.corp.google.com/1033044/diff/2001/1004#newcode1000 > Line 1000: fast_index = -2; // No need to lookup again > Named constant. > http://chromereview.prom.corp.google.com/1033044/diff/2001/1005 > File src/stub-cache.cc (right): http://chromereview.prom.corp.google.com/1033044/diff/2001/1005#newcode721 > Line 721: int total, fast, very_slow; > I would consider using counters for this; see src/v8-counters.h. http://chromereview.prom.corp.google.com/1033044/diff/2001/1005#newcode783 > Line 783: JSObject* holder; > This feels a like a weird mix of handles and non-handles. It might be the right > thing to do for performance reasons, but at least we should consider and > document why stuffing all the elements in handles isn't a good idea. http://chromereview.prom.corp.google.com/1033044/diff/2001/1005#newcode819 > Line 819: JSObject* recv = JSObject::cast(args[0]); > If you decide to use handles for the receiver and holder, you can use > args.at<JSObject>(0) to get a Handle<JSObject> back. http://chromereview.prom.corp.google.com/1033044/diff/2001/1005#newcode822 > Line 822: int fast_index = reinterpret_cast<int>(args[3]); > This should be passed on the stack as a smi so the garbage collectors doesn't > get confused. http://chromereview.prom.corp.google.com/1033044/diff/2001/1005#newcode829 > Line 829: case -1: > Named constants would be good for -1 and -2. http://chromereview.prom.corp.google.com/1033044 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
