Request for review - 8011268: NPG: Free unused VirtualSpaceNodes

[Coleen Phillimore]

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.


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

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


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

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