RFR(S) 8015255: NPG: Don't waste fragment at the end of a VirtualSpaceNode before retiring it.

[Jon Masamitsu]

On 10/17/13 1:01 PM, Mikael Gerdin wrote:
> Jon,
>
> Thanks for reviewing, see my reply inline:
>
> On 10/17/2013 06:09 PM, Jon Masamitsu wrote:
>> Looks good.  Some questions below.  No blockers to the integration.
>>
>>> 1120   for (int i = (int)MediumIndex; i >= (int)ZeroIndex; --i) {
>>> 1121     ChunkIndex index = (ChunkIndex)i;
>>
>> What do you think about code like this that uses ChunkIndex as the
>> for loop counter.
>>
>>    for (ChunkIndex i = Zer Index; i < NumberOfFreeLists; i =
>> next_chunk_index(i))
>>
>> I'm asking because I like the way it looks but it's not perfect.
>
> I had a go at doing what you suggested, but I didn't like it.
> I should probably change it to be similar to how you suggest, but I 
> ran into an issue with my prev_chunk_index where I didn't want to 
> return the value "-1", but "-1" is actually the loop exit condition.

Yah, returning "-1" is a odd value to return for a ChunkIndex.
You're call.


>
>>
>>> 3674     // The chunk sizes must be multiples of eachother, or this
>>> will fail
>>> 3675     STATIC_ASSERT(MediumChunk % SmallChunk == 0);
>>> 3676     STATIC_ASSERT(SmallChunk % SpecializedChunk == 0);
>>
>> The above is a requirement now?   I've never thought of it as a
>> requirement.  It just
>> happened to be the case.  I'm not opposed to it, just wondering.
>
> It's not a requirement per se, but the retire code and the test code 
> relies on this fact currently, so I want to make sure that it gets 
> caught if it changes.
>
>>
>> 3724       vsn.get_chunk_vs(MediumChunk + SpecializedChunk); //
>> Humongous chunks will be aligned up to MediumChunk + SpecializedChunk
>>
>> Should the chunk size beMediumChunk + SpecializedChunk  / 2
>> to test the alignment up?
>
> get_chunk_vs doesn't do the alignment up. I pass in a pre-aligned 
> value which should be equivalent to
> align_size_up(MediumChunk + 1, SpecializedChunk)

Ok.

Good to go then.

Jon

>
> /Mikael
>
>>
>> Jon
>>
>>
>>
>>
>>
>>
>> On 10/17/2013 5:56 AM, Mikael Gerdin wrote:
>>> Hi all,
>>>
>>> Please review this change to fix needless memory waste in Metaspace.
>>>
>>> Description of problem:
>>> When receiving an allocation request which cannot be satisfied with the
>>> current VirtualSpaceNode the remaining committed memory in the current
>>> node is
>>> wasted when a new node is created and the allocation is satisfied from
>>> the new
>>> node.
>>>
>>> Suggested fix:
>>> Before creating a new VirtualSpaceNode, "retire" the current node by
>>> allocating chunks of decreasing size from the committed memory present
>>> in the
>>> current node.
>>> To ensure that allocating chunks at the end of a node uses all 
>>> committed
>>> memory I've increased the size alignment for humongous chunks to the
>>> smallest
>>> chunk size. I don't foresee this increasing memory waste since
>>> previously the
>>> memory difference between 2-word-aligned sizes and smallest 
>>> chunk-aligned
>>> would go unused since no chunk would fit there.
>>>
>>> Testing:
>>> Added new unit tests for VirtualSpaceNode and the retire functionality.
>>> JPRT run (including the new unit tests)
>>>
>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8015255/webrev.0/
>>> Buglink: https://bugs.openjdk.java.net/browse/JDK-8015255
>>>
>>> /Mikael
>>
>