RFR(s): 8170520: Make Metaspace ChunkManager counters non-atomic
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Nov 30 13:30:02 UTC 2016
On Wed, Nov 30, 2016 at 2:22 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
wrote:
> Hi Thomas,
>
> On 2016-11-30 14:01, Thomas Stüfe wrote:
>
>> Hi Mikael,
>>
>> On Wed, Nov 30, 2016 at 1:41 PM, Mikael Gerdin <mikael.gerdin at oracle.com
>> <mailto: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
>> <https://bugs.openjdk.java.net/browse/JDK-8170520>
>>
>> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8170520-Make-
>> <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?
>>
>
> I ran an internal test suite, unfortunately.
> It basically runs the JCK, loading each test case in a new class loader,
> thereby triggering insane amounts of class unloading and metaspaces to be
> created.
>
>
Ah, that explains it. I tried jtreg runtime/Metaspace and
-XX:+ExecuteInternalVMTests, both ran fine. But we also run nightly jck
tests, so I'd like to check why I did not see an error. What was the exact
error you had, did the assert
in ChunkManager::locked_verify_free_chunks_total() trigger?
About the cleanup for ChunkManager::return_chunks(), should I do this as
part of this patch or as an extra patch?
>
>>
>> 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!
>>
>
> The test passed with my hack to fix the humongous chunks issue but I'll
> continue running further tests and report back if I see any further
> failures.
Thanks!
..Thomas
>
>
> /Mikael
>
>
>
>> ..Thomas
>>
>>
>> Thanks
>> /Mikael
>>
>>
>>
>> Small discussion here:
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2
>> 016-October/021709.html
>> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/
>> 2016-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
>>
>>
>>
More information about the hotspot-runtime-dev
mailing list