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

[Jon Masamitsu]

On 4/4/13 2:05 AM, Mikael Gerdin wrote:
> 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.

Fixed it in this patch.  It was simple enough that mercurial didn't reject
the following qpush :-)

Thanks again.

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