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

[Coleen Phillmore]

Thanks, Stefan.

On 9/20/2013 2:09 AM, Stefan Karlsson wrote:
> 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().

See below.   The _mdtype field is new.

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

Great.  We can remove that too with a different edit.

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

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