RFR(s): 8170520: Make Metaspace ChunkManager counters non-atomic
mikael.gerdin at oracle.com
Wed Nov 30 13:22:18 UTC 2016
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
> 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 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
> 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
> 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
> 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.
> Best Regards, Thomas
More information about the hotspot-runtime-dev