RFR(s): 8170520: Make Metaspace ChunkManager counters non-atomic
thomas.stuefe at gmail.com
Wed Nov 30 13:01:07 UTC 2016
On Wed, Nov 30, 2016 at 1:41 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
> 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-
> 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
> 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!
>> Small discussion here:
>> 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
>> 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
>> change can be pushed quickly once the 10 repo exists.
>> Best Regards, Thomas
More information about the hotspot-runtime-dev