Request for review - 8011268: NPG: Free unused VirtualSpaceNodes

[Jon Masamitsu]

On 4/4/13 5: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 :)

Yes, my bad.  I had intended to say something about that.
>
>>
>> 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().

Yes, that's why.  I kept all the methods for VirtualSpaceNode together 
so moved the
constructor down with the inc_container_count()/dec_container_count().
>
> 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.

I fixed that.

>
> 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
I think I will do this.  At one time I thought I would overlay 
_container_count
and _top but I don't need to save 1 or 2 words per 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.

A VirtualSpace that contains 1 Metachunk holds nothing else so that 
decrementing
the count on such a VirtualSpace will deallocate it.  I'll try it but 
may revert it.


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

Isn't incrementing the counter done by a VirtualSpaceNode
and decrementing done by a SpaceManager for the dec_container_count()
needs to be public?
>
>
> The class space only contains one VirtualSpaceNode currently, should 
> we add some code to make sure that we don't accidentally free that 
> memory?

Let me defer this because I think Coleen had something to say.

Thanks.

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