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

[Thomas Stüfe]

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
>>
>>
>>