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

Coleen Phillmore coleen.phillimore at
Fri Sep 20 04:54:55 PDT 2013

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 for your input,
> StefanK
>> Thanks,
>> Coleen
>> On 9/19/2013 3:48 PM, Stefan Karlsson wrote:
>>> In the fix for '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

More information about the hotspot-dev mailing list