Thanks a lot, Kasper!
http://codereview.chromium.org/155540/diff/10/12 File src/global-handles.cc (right): http://codereview.chromium.org/155540/diff/10/12#newcode98 Line 98: // At the same time deallocate all DESTROYED nodes On 2009/07/15 07:51:07, Kasper Lund wrote: > Terminate with . Done. http://codereview.chromium.org/155540/diff/10/12#newcode133 Line 133: // Reset all the lists On 2009/07/15 07:51:07, Kasper Lund wrote: > Terminate comment with . Done. http://codereview.chromium.org/155540/diff/10/11 File src/global-handles.h (right): http://codereview.chromium.org/155540/diff/10/11#newcode147 Line 147: class Node { On 2009/07/15 07:51:07, Kasper Lund wrote: > I wouldn't have all these functions inlined. It seems excessive. Move some of > them to the .cc file again. Done. But if feel that some methods should be moved too, or some brought back---please, let me know. http://codereview.chromium.org/155540/diff/10/11#newcode148 Line 148: public: On 2009/07/15 07:51:07, Kasper Lund wrote: > Indent public: one space. Done. http://codereview.chromium.org/155540/diff/10/11#newcode150 Line 150: void Initialize(Object* object) { On 2009/07/15 07:51:07, Kasper Lund wrote: > Move this down below the constructors and destructor. Done. http://codereview.chromium.org/155540/diff/10/11#newcode186 Line 186: // Accessors for next_. On 2009/07/15 07:51:07, Kasper Lund wrote: > next_ -> next I'd easily change it, but it might be design: method below provides various ways to access next_ field. http://codereview.chromium.org/155540/diff/10/11#newcode280 Line 280: Object* object_; // Storage for object pointer. On 2009/07/15 07:51:07, Kasper Lund wrote: > Why public? Class itself is private? http://codereview.chromium.org/155540/diff/10/11#newcode284 Line 284: enum State { On 2009/07/15 07:51:07, Kasper Lund wrote: > Move enum definitions to the start of the class def. Done. http://codereview.chromium.org/155540/diff/10/11#newcode291 Line 291: State state_; On 2009/07/15 07:51:07, Kasper Lund wrote: > Why are these public? I think for the same reason. http://codereview.chromium.org/155540/diff/10/11#newcode293 Line 293: private: On 2009/07/15 07:51:07, Kasper Lund wrote: > Weird indentation. Do you want me to fix it? http://codereview.chromium.org/155540/diff/10/11#newcode306 Line 306: public: On 2009/07/15 07:51:07, Kasper Lund wrote: > Weird indentation. Ditto. I hope I did screw it while copying around. http://codereview.chromium.org/155540/diff/10/11#newcode312 Line 312: static const int kNodesPerChunk = 10000; On 2009/07/15 07:51:07, Kasper Lund wrote: > This should be a power of two or a prime. Anything else seems wrong, and the > number seems a bit high. How about something like 1024? Sure. http://codereview.chromium.org/155540/diff/10/11#newcode318 Line 318: Chunk* first_; On 2009/07/15 07:51:07, Kasper Lund wrote: > You should definitely have a private: section (after public:) Done. http://codereview.chromium.org/155540/diff/10/11#newcode318 Line 318: Chunk* first_; On 2009/07/15 07:51:07, Kasper Lund wrote: > Do you really need two pointers? Why not have a pointer to the last chunk and > let the chunks refer to the previous chunk? Done. http://codereview.chromium.org/155540/diff/10/11#newcode321 Line 321: Node* last_available_; On 2009/07/15 07:51:07, Kasper Lund wrote: > It seems odd that last_available_ really isn't available because it's one past > the chunk? How about changing next_free_ and last_available_ to something like > next_ and limit_? Sure. http://codereview.chromium.org/155540/diff/10/11#newcode323 Line 323: public: On 2009/07/15 07:51:07, Kasper Lund wrote: > Again. Done. http://codereview.chromium.org/155540/diff/10/11#newcode325 Line 325: if (next_free_ < last_available_) { On 2009/07/15 07:51:07, Kasper Lund wrote: > I would re-arrange this code to have the fast case inlined, but the slow-case > with the new Chunk out of line. Done. http://codereview.chromium.org/155540/diff/10/11#newcode342 Line 342: ASSERT(current); // At least a single block must be allocated On 2009/07/15 07:51:07, Kasper Lund wrote: > current != NULL Done. http://codereview.chromium.org/155540/diff/10/11#newcode343 Line 343: while (true) { On 2009/07/15 07:51:07, Kasper Lund wrote: > do { ... } while (current != NULL)? Done. http://codereview.chromium.org/155540/diff/10/11#newcode346 Line 346: if (!next) { On 2009/07/15 07:51:07, Kasper Lund wrote: > Get rid of the break stuff and use do-while? Done. http://codereview.chromium.org/155540/diff/10/11#newcode356 Line 356: Pool() { On 2009/07/15 07:51:07, Kasper Lund wrote: > Constructors before methods. Done. http://codereview.chromium.org/155540/diff/10/11#newcode357 Line 357: first_ = current_ = new Chunk(); On 2009/07/15 07:51:07, Kasper Lund wrote: > Could this be avoided? Why not just let the first Allocate call do the chunk > allocation and avoid repeating the code here (you'll also avoid a complicated > static constructor)? You may have to change Release to allow not having any > chunks, but that's a minor thing. If you just NULL out the fields here, it > should just work, no? I hope it will. I just wanted to warm it up to save couple of cycles on first allocation. If you think, it doesn't worth it, I'd remove. http://codereview.chromium.org/155540/diff/10/11#newcode364 Line 364: static Pool pool; On 2009/07/15 07:51:07, Kasper Lund wrote: > Why not pool_? Done. http://codereview.chromium.org/155540/diff/10/11#newcode385 Line 385: // Free least of deallocated nodes. Even though they are deallocated, On 2009/07/15 07:51:07, Kasper Lund wrote: > least -> list Done. http://codereview.chromium.org/155540 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
