Request for review - 8011268: NPG: Free unused VirtualSpaceNodes

Fri Apr 5 12:01:27 UTC 2013

On 4/5/2013 6:45 AM, Mikael Gerdin wrote:
>
>
> 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.

He should merge before moving this because I think a trivial mercurial 
merge might miss these changes, or mercurial might barf on the whole 
thing.  I don't know.  Just a warning.  I don't mind that it's moved.

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

Yes, I agree with you, placement new would be a lot nicer and then the 
constructor would be called.   Right now it isn't and it's a bit 
confusing having a way to initialize these that you think might be 
called.  (I know you didn't say this, I'm piling, I mean adding, on).

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

Ok. I didn't read the counter code so didn't match your comment.
>
>>
>>
>> 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.

Maybe, I guess.   What were the tracing results of this change? (ques 
for Jon).   Do we see these things going away on our favorite nashorn 
application?

thanks,
Coleen

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