Request for review - 8011268: NPG: Free unused VirtualSpaceNodes

[Jon Masamitsu]

Coleen,

Thanks for taking a look.

On 4/4/2013 3:41 PM, 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!

Yes, but it doesn't do as good a job either.

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

I'll move it back.

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

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.
>
Yes, there we two.  I'm following Mikael and your suggestion setting the 
container at
allocation.

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

Fixed.

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

That code is gone now.

When I changed the ChunkList to be FreeList<Metachunk> the list became 
doubly linked
so I didn't have to walk the whole list to remove a chunk.

>
> This is a nit of mine.   Can you move the declaration at 805 to 807 
> where the initial value is returned?

Done.

Thanks.

Jon

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