I'm not sure I understand why you need two free lists? I would try to simplify it more and have the chunky pool and a single free list.
First round of comments: 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 Terminate with . http://codereview.chromium.org/155540/diff/10/12#newcode133 Line 133: // Reset all the lists Terminate comment with . 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 { I wouldn't have all these functions inlined. It seems excessive. Move some of them to the .cc file again. http://codereview.chromium.org/155540/diff/10/11#newcode148 Line 148: public: Indent public: one space. http://codereview.chromium.org/155540/diff/10/11#newcode150 Line 150: void Initialize(Object* object) { Move this down below the constructors and destructor. http://codereview.chromium.org/155540/diff/10/11#newcode186 Line 186: // Accessors for next_. next_ -> next http://codereview.chromium.org/155540/diff/10/11#newcode280 Line 280: Object* object_; // Storage for object pointer. Why public? http://codereview.chromium.org/155540/diff/10/11#newcode284 Line 284: enum State { Move enum definitions to the start of the class def. http://codereview.chromium.org/155540/diff/10/11#newcode291 Line 291: State state_; Why are these public? http://codereview.chromium.org/155540/diff/10/11#newcode293 Line 293: private: Weird indentation. http://codereview.chromium.org/155540/diff/10/11#newcode306 Line 306: public: Weird indentation. http://codereview.chromium.org/155540/diff/10/11#newcode312 Line 312: static const int kNodesPerChunk = 10000; 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? http://codereview.chromium.org/155540/diff/10/11#newcode318 Line 318: Chunk* first_; You should definitely have a private: section (after public:) http://codereview.chromium.org/155540/diff/10/11#newcode318 Line 318: Chunk* first_; Do you really need two pointers? Why not have a pointer to the last chunk and let the chunks refer to the previous chunk? http://codereview.chromium.org/155540/diff/10/11#newcode321 Line 321: Node* last_available_; 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_? http://codereview.chromium.org/155540/diff/10/11#newcode323 Line 323: public: Again. http://codereview.chromium.org/155540/diff/10/11#newcode325 Line 325: if (next_free_ < last_available_) { I would re-arrange this code to have the fast case inlined, but the slow-case with the new Chunk out of line. http://codereview.chromium.org/155540/diff/10/11#newcode342 Line 342: ASSERT(current); // At least a single block must be allocated current != NULL http://codereview.chromium.org/155540/diff/10/11#newcode343 Line 343: while (true) { do { ... } while (current != NULL)? http://codereview.chromium.org/155540/diff/10/11#newcode346 Line 346: if (!next) { Get rid of the break stuff and use do-while? http://codereview.chromium.org/155540/diff/10/11#newcode356 Line 356: Pool() { Constructors before methods. http://codereview.chromium.org/155540/diff/10/11#newcode357 Line 357: first_ = current_ = new Chunk(); 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? http://codereview.chromium.org/155540/diff/10/11#newcode364 Line 364: static Pool pool; Why not pool_? http://codereview.chromium.org/155540/diff/10/11#newcode385 Line 385: // Free least of deallocated nodes. Even though they are deallocated, least -> list http://codereview.chromium.org/155540 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
