Request for review - 8011268: NPG: Free unused VirtualSpaceNodes

[Mikael Gerdin]

On 2013-04-05 00:41, Coleen Phillimore wrote:
>
> Hi Jon,
>
> Mikael's review is more thorough than what I've looked at so far.  I
> wanted to add to it and my own comments at the end.   I didn't realize
> you were working on this.  This is far less code than changing our
> allocation scheme to malloc!
>
> On 04/04/2013 08:28 AM, Mikael Gerdin wrote:
>> Jon,
>>
>> On 2013-04-04 06:00, Jon Masamitsu wrote:
>>> After class unloading deallocate any VirtualSpaceNodes not being used.
>>
>> The webrev is a bit confusing since it says for metaspace.cpp:
>> rev 4373 : 8011173: NPG: Replace the ChunkList implementation with
>> class FreeList<Metachunk>
>> Reviewed-by: mgerdin
>>
>> this implies to me that the 8011173 patch is included in this webrev
>> when in fact this webrev is incremental on top of that patch.
>> I don't know if that was intentional or not but it confused me a bit
>> when trying to apply this patch.
>>
>> I'm totally fine with incremental webrevs though, just as long as they
>> are advertised as such :)
>>
>>>
>>> When the classes for a dead class loader are unloaded, all the
>>> Metachunks
>>> that had been used by that class loader are put on the Metachunk
>>> free list for later reuse.   If all the Metachunks allocated out of a
>>> VirtualSpaceNode are on the Metachunk free list, remove the Metachunks
>>>   from the free list and deallocate the VirtualSpaceNode.
>>>
>>> The constructor for VirtualSpaceNode was moved to be below the
>>> definition of
>>> SpaceManager.
>>
>> I'm not sure why you moved the VirtualSpaceNode constructor to after
>> the definition of SpaceManager, but I see that
>> {inc,dec}_container_count need to reference the expand_lock().
>>
>
> You moved the constructor for VirtualSpaceNode but the gc baseline
> doesn't have changes I made to it recently, so moving it will not get
> the most recent changes from hotspot-rt.

FWIW those changes are actually in hotspot-gc so Jon will need to merge 
with them before pushing.

>
>
>> Also, when I applied your patch and looked at the code in eclipse it
>> helpfully notified me that you are forgetting to initialize
>> _container_count in the VirtualSpaceNode constructor that you moved.
>>
>> Why don't you set the Metachunk::_container field in
>> Metachunk::initialize():
>>
>>  776   // Point the chunk at the space
>>  777   Metachunk* result = Metachunk::initialize(chunk_limit,
>> chunk_word_size);
>>  778   return result;
>
> In fact, I believe the Metachunk constructor is never called. Pointers
> are transformed into these.  If that's the case the Metachunk
> constructors should be removed.

I don't see where I said anything about the Metachunk constructor.

But the line where we call Metachunk::initialize is where we initialize 
the memory that we allocated for the chunk is set up.

I would prefer it if we could replace this with a call to the placement 
new operator and use a constructor to set up the Metachunk object but 
I'm not going to go into that argument.

>
>>
>>  It's called from VirtualSpaceNode so you should be able to pass
>> "this" along to Metachunk::initialize. That will save us the walking
>> of VirtualSpaceList to find the correct node to increment the count for.
>>
>> If you set up Metachunk::_container in Metachunk::_initialize you
>> could also skip the wrapper for ChunkManager::chunk_freelist_allocate
>> inside VirtualSpaceList and just add:
>>
>> if (next != NULL) {
>>   next->container()->inc_container_count();
>> }
>> at the end of VirtualSpaceList::get_new_chunk
>>
>> You are not decreasing the _container_count when returning humongous
>> chunks in ~SpaceManager:
>> 2123     Metachunk* next_humongous_chunks = humongous_chunks->next();
>> // here you could do a
>> humongous_chunks->container()->dec_container_count();
>> 2124
>> chunk_manager->humongous_dictionary()->return_chunk(humongous_chunks);
>>
>> You are already incrementing _container_count for humongous chunks
>> allocated from the hum chunk free list because those allocations go
>> through the chunk_freelist_allocate wrapper function you added.
>>
>> It also feels strange that inc_container_count() is private but
>> dec_container_count and container_count() are public. I realize that
>> it works currently because VirtualSpaceList is a friend of
>> VirtualSpaceNode but it seems like a strange level of encapsulation.
>>
>>
>> The class space only contains one VirtualSpaceNode currently, should
>> we add some code to make sure that we don't accidentally free that
>> memory?
>>
>
> The class space can contain >1 virtual space nodes if
> !UseCompressedKlassPointers right now so you can purge unused spaces.
> I'm making this go away.
>
> It looks like you find the containing virtual space node when you
> deallocate the chunks which looks really expensive.   Why not have all
> the chunks have a field with their container when they are created or
> reused?  Am I reading this wrong?   These two "contains()" calls at
> lines 2033, 2034 are going to be killer.

This is exactly what I was saying above:

 >> Why don't you set the Metachunk::_container field in
 >> Metachunk::initialize():
 >>
 >>  776   // Point the chunk at the space
 >>  777   Metachunk* result = Metachunk::initialize(chunk_limit,
 >> chunk_word_size);

We could easily pass on the VirtualSpaceNode to Metachunk::initialize.


>
>
> At 932, you should not purge the current virtual space, I think, because
> you might just allocate it again.

There might be loads and loads of free chunks in the other virtual 
spaces and we may be recovering from a spike in metaspace allocation.
If we are able to release metaspace memory I think we should.
It feels pretty unlikely that we'll end up with a VirtualSpaceNode with 
a count of 0 so we should take every chance we get to free one up IMO.

/Mikael

>
> Around 928, why do you have to walk the virtual space itself?  Why not
> walk the linked list of free chunks for the container entry that matches
> the virtual space node if the virtual space count is null? You could do
> one walk and remove all entries with all virtual space nodes that are
> matching if you are worried about the loop in the loop.   It has to find
> the chunk in the free list anyway to remove it.
>
> This is a nit of mine.   Can you move the declaration at 805 to 807
> where the initial value is returned?
>
> Thanks!
> Coleen
>
>> /Mikael
>>
>>>
>>> http://cr.openjdk.java.net/~jmasa/8011268/webrev.00/
>>>
>>> Thanks.
>>>
>>> Jon
>