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

[Mikael Gerdin]

Jon,

On 2013-04-02 17:43, Jon Masamitsu wrote:
> 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.

I just noticed that you forgot to remove the comment

// ChunkList methods
at line 1385.
Fix it in this push or fix it with 8011268, I'm fine either way.

/Mikael

>
>>
>> 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