Review request: 8025096: Move the ChunkManager instances out of the VirtualSpaceLists

[Stefan Karlsson]

On 9/20/13 4:14 AM, Coleen Phillmore wrote:
>
> Stefan,
>
> Thanks for sending this to hotspot-dev.   This change seems okay but 
> the original intent was that the free chunks went along with their 
> virtual space list, which manage the list of mmap spaces. So that's 
> why they were contained by VirtualSpaceList.  And ChunkManager was 
> more hidden - nobody was supposed to know about it.

But that's not what the code looked like. All over the place you called 
vs_list()->chunk_manager()->... On only a few places where ChunkManager 
really used directly by the VirtualSpaceList.

>
> Now all the SpaceManagers have an extra pointer, and there are two 
> space managers per class loader in the case of compressed class 
> pointers, so that could add up to an interesting amount of extra 
> memory, especially for 2M anonymous classes.

I see your point. But if that memory wastage is that important, then I 
don't understand why the SpaceManager has a VirtualSpaceList* _vs_list 
field, when the correct VSL can be fetched globally as you suggest below 
that I do with SpaceManager::chunk_manager().

There are even more space savings to be made in the SpaceManager class. 
The _medium_chunk_bunch isn't used at all, for example.

>
> Since SpaceManager has a back pointer to VirtualSpaceList, you could 
> still have ChunkManager belong to VSL and indirectly get them:
>     SpaceManager::chunk_manager() { return _vs_list->chunk_manager(); }
>
> Of if you don't like that, you could have:
>
>     SpaceManager::chunk_manager() { return *_mdtype == 
> Metaspace::ClassType ? _chunk_manager_class : the other one; }

Sure. I can do that, but then I would like to do the same for the 
VirtualSpaceList* _vs_list.

>
> *The C heap allocation looks a lot cleaner, and the change to 
> get_new_chunk, which is the main purpose of this change looks really 
> good.   I'm just worried about the extra footprint and there is 
> already _vs_list.   Maybe neither is needed and could be fetched by 
> _mdtype, which wasn't there when _vs_list was added. 

Yes.

> Can you adjust your change to not add this memory?   Or file a new bug 
> to remove the additional footprint after this?

I'll move out _chunk_manager and _vs_list from SpaceManager, and send 
out a new review request later today.

thanks for your input,
StefanK
>
> Thanks,
> Coleen
>
> On 9/19/2013 3:48 PM, Stefan Karlsson wrote:
>> http://cr.openjdk.java.net/~stefank/8025096/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8025096
>>
>> In the fix for 'JDK-8024547 
>> <https://bugs.openjdk.java.net/browse/JDK-8024547>: MaxMetaspaceSize 
>> should limit the committed memory used by the metaspaces' we need to 
>> make a clearer distinction between when new memory is committed and 
>> when memory is allocated inside already committed memory.
>>
>> Moving the ChunkManager, which handles the free chunk list, out of 
>> the VirtualSpaceList, which handles the committed metaspace memory, 
>> is a step in that direction.
>>
>> Changes:
>> - Move the ChunkManager instances from VirtualSpaceList to SpaceManager.
>> - Move the inc_container_count() into ChunkManager::free_chunks_get. 
>> Now the Chunk manager calls dec_container_count when chunks are 
>> inserted in the chunk free list and inc_container_count when chunks 
>> are removed from the chunk free list.
>> - Moved the ChunkManager::chunk_freelist_allocate() call out from the 
>> VirtualSpaceList::get_new_chunk()
>>
>> thanks,
>> StefanK
>