Issue 155540: Fasten global handles allocation.

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+281 lines, -192 lines)			Patch
	M	src/global-handles.h	View	1 2 3 4	3 chunks	+180 lines, -2 lines	0 comments	Download
	M	src/global-handles.cc	View	1 2 3 4	6 chunks	+101 lines, -190 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Kasper Lund

I'm not sure I understand why you need two free lists? I would try to ...

11 years, 5 months ago (2009-07-15 07:51:07 UTC) #2

antonm

11 years, 5 months ago (2009-07-15 11:12:58 UTC) #3

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.

Expand Messages | Collapse Messages