Request for review (s) - NPG: Replace the ChunkList implementation with class FreeList<Metachunk>

[Jon Masamitsu]

Mikael,

Thanks for the review.

On 04/02/13 07:42, Mikael Gerdin wrote:
> Jon,
>
> On 2013-04-02 00:13, Jon Masamitsu wrote:
>> For the perm gen removal project the ChunkList class
>> which implemented a simple freelist of Metachunk's was added.
>> This change replaces ChunkList with FreeList<Metachunk>.
>>
>> Changes include
>>
>> - Deletion of ChunkList
>> - Code to initialize the freelists in the FreeList style
>> - Replacement of call to ChunkList methods with the
>> equivalent calls to FreeList methods.
>>
>> http://cr.openjdk.java.net/~jmasa/8011173
>
> This looks like a good cleanup step.
> I'm fine with you pushing this as is, these are just my thoughts about 
> the FreeList<> stuff.

I have some other changes build on top these changes so I would
like to push them with minimal changes.

>
> Regarding your comment in ChunkManager::return_chunks I think it could 
> be resolved by making SpaceManager::_chunks_in_use[] into an array of 
> ChunkList, similar to ChunkManager::_free_chunks.

I think something like this would work but as you note below FreeList is 
not really a freelist but
just a list.  I hesitated to make _chunks_in_use a FreeList because it 
would confuse the casual
reader.
>
> The name FreeList is kind of a misnomer since it's actually just the 
> head of a generic linked list.

Yes the name is wrong.  There is actually a prepend() method that takes 
a FreeList and that
is what I wanted to use  but the name "FreeList" stopped me.

>
> Maybe we should consider encapsulating the managing of the _next and 
> _prev fields only through the FreeList<> template by making the 
> setters to them private and friend:ing FreeList<Metachunk> in class 
> Metachunk and only ever letting FreeList manage the links?

So I don't understand that.  I've created a CR for your suggested clean ups.
8011265   I'll think about that when I get to fixing this.

>
> It's confusing to have FreeList handle the links through link_prev and 
> link_next and other parts of the code using set_next and set_prev.

link_prev() and link_next() have special meaning in CMS.  One or the 
other used to set
the is_free bit in a CMS freelist.  It would be good to clean that up.

Thanks again.

Jon

>
> /Mikael
>
>
>>
>> Thanks.
>>
>> Jon