Request for review - 8011268: NPG: Free unused VirtualSpaceNodes

[Mikael Gerdin]

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

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;

  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?

/Mikael

>
> http://cr.openjdk.java.net/~jmasa/8011268/webrev.00/
>
> Thanks.
>
> Jon