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

[Mikael Gerdin]

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.

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

/Mikael

>
> ..Thomas
>
>
>     Thanks
>     /Mikael
>
>
>
>         Small discussion here:
>         http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-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
>
>