http://codereview.chromium.org/140056/diff/1/4 File src/compilation-cache.cc (right):
http://codereview.chromium.org/140056/diff/1/4#newcode37 Line 37: #define NUMBER_OF_SUB_CACHES 4 On 2009/06/22 09:49:39, Kasper Lund wrote: > Why not use constants for this? I would definitely have something like enum { > kNumberOfSubCaches = 4, ... } or static const ints. Done. http://codereview.chromium.org/140056/diff/1/4#newcode39 Line 39: // The number of generations for each sub cache. On 2009/06/22 09:49:39, Kasper Lund wrote: > This should probably also be constants. Done. http://codereview.chromium.org/140056/diff/1/4#newcode52 Line 52: static CompilationSubCache* entry[NUMBER_OF_SUB_CACHES] = On 2009/06/22 09:49:39, Kasper Lund wrote: > entry seems like a weird name. How about subcaches? Done. http://codereview.chromium.org/140056/diff/1/4#newcode73 Line 73: static const int kInitialCacheSize = 64; On 2009/06/22 09:49:39, Kasper Lund wrote: > Maybe move this constant out to the other constant definitions? Done. http://codereview.chromium.org/140056/diff/1/4#newcode87 Line 87: for (int generation = generations_ - 1; On 2009/06/22 09:49:39, Kasper Lund wrote: > I would probably rewrite this to use |i| as the counter (like it's done in > CompilationSubCache::Clear). Done. http://codereview.chromium.org/140056/diff/1/4#newcode210 Line 210: Handle<CompilationCacheTable> table = GetTable(generation); On 2009/06/22 09:49:39, Kasper Lund wrote: > Weird indentation. Done. http://codereview.chromium.org/140056/diff/1/4#newcode249 Line 249: for (generation = 0; generation < generations(); generation++) { On 2009/06/22 09:49:39, Kasper Lund wrote: > Weird indentation. Done. http://codereview.chromium.org/140056/diff/1/2 File src/compilation-cache.h (right): http://codereview.chromium.org/140056/diff/1/2#newcode96 Line 96: // The compilation cache consists of several generational sub-caches which uses On 2009/06/22 09:49:39, Kasper Lund wrote: > Could all this CompilationSubCache stuff be moved to compilation-cache.cc? It > seems like it's an implementation detail for the CompilationCache. Done. http://codereview.chromium.org/140056/diff/1/2#newcode103 Line 103: explicit CompilationSubCache(int generations): generations_(generations) { On 2009/06/22 09:49:39, Kasper Lund wrote: > Space before :? Done. http://codereview.chromium.org/140056/diff/1/2#newcode104 Line 104: // tables_ = new Object*[generations]; On 2009/06/22 09:49:39, Kasper Lund wrote: > Remove. Done. http://codereview.chromium.org/140056/diff/1/2#newcode107 Line 107: virtual ~CompilationSubCache() { On 2009/06/22 09:49:39, Kasper Lund wrote: > Why is this virtual? Is it ever called and needed? It is never called with the current static allocation if the sub-caches - removed. http://codereview.chromium.org/140056 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
