Request for review - 8011268: NPG: Free unused VirtualSpaceNodes

Fri Apr 5 15:42:16 UTC 2013

On 4/5/2013 5:01 AM, Coleen Phillimore wrote:
> 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.

I'll merge and look hard at the constructor to be sure I get your changes.

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

On the way.

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

I ran runThese -quick that I know has at some point lots of excess 
capacity.  Nashorn as I
recall has Metaspace usage that rises and then levels off and stays at 
the level
(keeps freeing and reallocating metadata so no excessive capacity that 
is not
used).

Jon

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