Allocate global handles in chunks to make it faster.

Allocate global handles in chunks to make it faster.

[antonm, 2009-10-23 04:20:59]

Guys,

returning to old CL.  I found that for some modification tests which create a
lot of wrappers, allocating of global handle is a bottleneck thus reviving this
CL.

I know that Vitaly has even more sophisticated one.  I tried to patch it, but
global handles diverged too much, so I think Vitaly is in a better position to
merge the stuff.

I'd prefer to submit my CL as that would make it easier for us to compare
performance implications.

[Mads Ager (chromium), 2009-10-23 12:45:21]

LGTM with comments.

The nodes per chunk number seems high.  Isn't this going to waste too much space
for most web pages?

This is getting complicated.  I think we should be able to come up with
something simpler than this?  Maybe splitting this one list into multiple lists
or something.  Feel free to consider that in a separate change.

http://codereview.chromium.org/327008/diff/1/3
File src/global-handles.cc (right):

http://codereview.chromium.org/327008/diff/1/3#newcode236
Line 236: static const int kNodesPerChunk = (1 << 13) - 1;
This seems like a lot of nodes per chunk?  What is this number based on?

http://codereview.chromium.org/327008/diff/1/2
File src/global-handles.h (right):

http://codereview.chromium.org/327008/diff/1/2#newcode153
Line 153: // List of deallocated nodes. Even though they are deallocated,
Could you update this comment with a bit more information.  It took me a while
to figure out the way |first_deallocated_| is always the node before |head_|. 
This deallocated list is an unused prefix of the list started at |head_| where
|first_deallocated_| points to the end of the deallocated list.

[antonm, 2009-10-23 13:04:25]

Mads, thanks a lot for comments.

http://codereview.chromium.org/327008/diff/1/3
File src/global-handles.cc (right):

http://codereview.chromium.org/327008/diff/1/3#newcode236
Line 236: static const int kNodesPerChunk = (1 << 13) - 1;
On 2009/10/23 12:45:21, Mads Ager wrote:
> This seems like a lot of nodes per chunk?  What is this number based on?

No serious real reasons.  Two factors: nodes are of 40 bytes for 32bit, so it's
320K (unless there are additional alignments), probably affordable on all
machines even if it'd be 1M.

Two: couple of times I saw numbers like 10.000s of global handles, so 8191
looked like a 'pragmatic' approximation.

Once again, no hard data.

http://codereview.chromium.org/327008/diff/1/2
File src/global-handles.h (right):

http://codereview.chromium.org/327008/diff/1/2#newcode153
Line 153: // List of deallocated nodes. Even though they are deallocated,
On 2009/10/23 12:45:21, Mads Ager wrote:
> Could you update this comment with a bit more information.  It took me a while
> to figure out the way |first_deallocated_| is always the node before |head_|. 
> This deallocated list is an unused prefix of the list started at |head_| where
> |first_deallocated_| points to the end of the deallocated list.

Sure.  Is it good enough?

[Mads Ager (chromium), 2009-10-26 06:44:53]

LGTM

The new comment is helpful, thanks.

We should measure the impact of different sizes of chunks.  If we can save some
memory with little or no performance cost we should.

http://codereview.chromium.org/327008/diff/5001/6001
File src/global-handles.h (right):

http://codereview.chromium.org/327008/diff/5001/6001#newcode157
Line 157: // ny |next_free|:
ny -> by

[antonm, 2009-10-26 11:53:35]

Halving the amount of handles per chunk and if performance would be fine,
submitting this.

Thanks a lot for review, Mads.

http://codereview.chromium.org/327008/diff/5001/6001
File src/global-handles.h (right):

http://codereview.chromium.org/327008/diff/5001/6001#newcode157
Line 157: // ny |next_free|:
On 2009/10/26 06:44:53, Mads Ager wrote:
> ny -> by

Done.