RFR(s): 8170520: Make Metaspace ChunkManager counters non-atomic

[Thomas Stüfe]

Hi Mikael,

On Wed, Nov 30, 2016 at 1:41 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
wrote:

> Hi Thomas,
>
> On 2016-11-30 08:43, Thomas Stüfe wrote:
>
>> Hi all,
>>
>> please take a look at this small fix. (This is one of the smaller fixes in
>> preparation for a prototype for JDK-8166690, I try to move the smaller
>> stuff out of the way first).
>>
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8170520
>>
>> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8170520-Make-
>> Metaspace-ChunkManager-Counters-NonAtomic/webrev.00/webrev/
>>
>
> I threw some testing at your change and it seems like you've forgotten
> about calling inc_free_chunks_total for the humongous chunks in
> ~SpaceManager. I'm going to run more tests on a patched build where I just
> hack in a call to inc_free_chunks_total for reach humongous chunk.
>
>
Ouch, I think you are a right, I forgot about that case :( What tests did
you run?


> It would be great if you could have a look at unifying the
> ChunkManager::return_chunks API for standard and humongous chunks so that
> we wouldn't need the special casing for this in ~SpaceManager
> That way we could also make humongous_dictionary() private to ChunkManager
> which would improve encapsulation.
>
>
Yes, this makes sense, I will do that. Thank you for testing!

..Thomas


> Thanks
> /Mikael
>
>
>
>> Small discussion here:
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2
>> 016-October/021709.html
>>
>> Background: ChunkManager keeps counters (_free_chunks_count and
>> free_chunks_total). These counters are updated using Atomics. Because this
>> is expensive, code goes thru some lengths to accumulate updates, which in
>> turn makes the code more complicated and error prone as necessary. But
>> updating the counters atomically is not needed, because updates happen
>> under lock protection anyway.
>>
>> The proposed fix makes updates non-atomic and moved the call to
>> inc_free_chunks_total() into ChunkManager::return_chunks() - close to
>> where
>> the chunk is actually returned to the freelist, so the time window where
>> the counters are invalid is very small now.
>>
>> It also fixes an assert and makes inc_free_chunks_total() private because
>> now it can be private.
>>
>> Note that I do not intend to push this into 9, so this is for the upcoming
>> 10 repository. I would like to get some reviews nevertheless, so this
>> small
>> change can be pushed quickly once the 10 repo exists.
>>
>> Thanks!
>>
>> Best Regards, Thomas
>>
>>