RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

Thomas Stüfe thomas.stuefe at gmail.com
Fri Aug 21 04:42:03 UTC 2020


Hi Coleen,

great, thanks for your three reviews!

Nothing much to say really, makes all sense. I'll prepare a new webrev with
yours and Leo's feedback incorporated, and whoever else chimes in until
then.

Cheers, Thomas

On Thu, Aug 20, 2020 at 3:48 PM Coleen Phillimore <
coleen.phillimore at oracle.com> wrote:

>
> Hi Thomas,
>
> Here are my comments from reading the jtreg test files:
>
> === MetaspaceGTests.java
>
> /*
>   * Note: This runs the metaspace-related parts of gtest in
> configurations which
>   *  are not tested explicitely in the standard gtests.
>   *
>   */
>
> explicitly typo.
>
> This is an interesting change.  I don't think there's another way to do
> this.  Others might have other suggestions.
>
> === MetaspaceTestContext.java
>
> Missing copyright.
> There are two classes called this (or close to this). I think the gtest
> version should be called (gtest) Helper, just to
> avoid confusion.
>
> === TestMetaspaceAllocation*
>
>   * @test id=debug
>
> Is this something in the version of jtreg we test?
>
> These tests need @bug 8251158
> even though it's not a bug.
>
> The tests look really good.  They cover and stress metaspace way more
> than all of our indirect testing.
>
> Coleen
>
> On 8/12/20 1:59 PM, Thomas Stüfe wrote:
> > Dear all,
> >
> > I would like to start the review for the implementation of JEP 387
> "Elastic
> > Metaspace".
> >
> > The relevant JBS items are:
> >
> > JEP: https://openjdk.java.net/jeps/387
> >
> > Implementation Issue (pretty much only a placeholder currently):
> > https://bugs.openjdk.java.net/browse/JDK-8251158
> >
> > --
> >
> > Reminder of why we do this:
> >
> > 1. The new metaspace saves memory. How much depends on context: if it is
> > not a problem today it will continue to be none. But in cases where today
> > we face large metaspace consumption we may get reductions, sometimes
> > drastic ones. These reductions are caused by two facts:
> > - after mass class unloading we release memory more promptly to the OS
> > - even without class unloading chunk allocation is just plain smarter,
> > which reduces the overhead per class loader. This is especially true for
> > scenarios involving masses of small loaders which only load very few
> > classes.
> >
> > As an example, see [1] where two VMs - one stock, one patched - run the
> > same test program which creates tons of small loaders. The second run
> shows
> > a much reduced memory footprint and increased elasticity.
> >
> > 2. The rewritten metaspace code base got cleaner and less complex and
> thus
> > should be much easier to maintain and to extend. It also would be
> possible
> > to easily reuse the allocator for different parts of the VM, since it is
> > less tightly tied to the notion of just storing class metadata.
> >
> > --
> >
> > In preparation of this review I prepared a guide [2], [3], which gives an
> > architectural overview and should be the starting point for an interested
> > Reviewer.
> >
> > The prototype has been tested extensively for quite some time in SAP's CI
> > system. We regularly run JCK test, JTReg tests and a whole battery of SAP
> > internal tests on 8 platforms. Tests are also currently ongoing at Oracle
> > and Red Hat.
> >
> > --
> >
> > The full webrev [4] is somewhat large. In order to make this more
> bearable
> > I broke the patch up into three parts:
> >
> > 1) The core parts [5]
> >
> > This is the heart of the metaspace, mostly rewritten from scratch. I
> > propose any Reviewer should not look at the diff but rather just examine
> > the new implementation. One possible exception is metaspace.hpp, which is
> > the outside interface to metaspace.
> >
> > There are several ways to get a feeling for this code (after reading at
> > least the concept part of the provided guide [2]). The central, most
> > "beefy" classes are:
> >
> > - VirtualSpaceNode - does all the work of reserving, committing,
> > uncommitting memory
> > - RootChunkArea - does the grunt work of chunk splitting and merging
> > - ChunkManager - which takes care of the chunk freelists, of directing
> > chunk splits and merges, of enlarging chunks in place
> > - MetaspaceArena - which takes care of fine granular allocation on behalf
> > of a CLD, and of managing deallocated blocks.
> >
> > One way to review could be bottom up: starting at
> > VirtualSpaceNode/CommitMask/RootChunkArea(LUT), working your way up to
> > ChunkManager and Metachunk; finally up to to MetaspaceArena while taking
> a
> > side stroll to FreeBlocks/BinList/BlockTree.
> >
> > Another way would be to follow typical paths:
> >
> > Allocation path: Starting at MetaspaceArena::allocate() ->
> > Metachunk::allocate() (note the committing-on-demand code path starting
> > here) -> ChunkManager::get_chunk() ->
> > VirtualSpaceList/Node->allocate_root_chunk().
> >
> > Destruction: ~MetaspaceArena() -> ChunkManager::return_chunk() ->
> (merging
> > chunks) -> (uncommitting chunks)
> >
> > Premature deallocation: -> MetaspaceArena::deallocate() -> see freeblock
> > list handling
> >
> > 2) The tests [6]
> >
> > This part of the patch contains all the new tests. There are a lot; the
> > test coverage of the new metaspace is very thorough.
> >
> > New gtests have been added under 'test/hotspot/gtest/metaspace'.
> > New jtreg have been added under
> > 'test/hotspot/jtreg/runtime/Metaspace/elastic' and under
> > 'test/hotspot/jtreg/gtest/MetaspaceGtests.java'.
> >
> > 4) Miscellaneous diffs [7]
> >
> > This is the part of the patch which is neither considered core nor test.
> > These changes are small, often uninteresting, and can be reviewed via
> diff.
> >
> > ---
> >
> > Out of scope for this patch is revamping outside metaspace statistics:
> > monitoring of metaspace statistics is mostly left untouched, beyond the
> > work needed to keep existing tests green. I wanted to keep those changes
> > separate from the core changes for JEP387. They are tracked in
> JDK-8251392
> > [8] and JDK-8251342 [9], respectively.
> >
> > ---
> >
> > Code complexity:
> >
> > Numerically, lines of code went ever so slightly down with this patch:
> >
> > Before: (cloc hotspot/share/memory)
> >
> -------------------------------------------------------------------------------
> > C++ 36 2533 3097 12735
> > C/C++ Header 54 1728 2867 6278
> >
> -------------------------------------------------------------------------------
> > SUM: 90 4261 5964 19013
> >
> > After:
> >
> -------------------------------------------------------------------------------
> > C++ 50 3048 3774 13127
> > C/C++ Header 63 2006 3575 5738
> >
> -------------------------------------------------------------------------------
> > SUM: 113 5054 7349 18865
> >
> > But since the new implementation is more powerful than the old one -
> doing
> > things like committing/uncommitting on demand, guarding allocated blocks
> > etc - one could argue that the new solution does quite a lot more with
> > slightly less code, which I think is a good result.
> >
> > ---
> >
> > Idle musing: it would simplify matters quite a bit if we were to unify
> > class space and non-class metaspace. If we keep the current narrow Klass
> > pointer encoding scheme, we would still need to keep the space they are
> > stored in contiguous. But we could use the class space for everything, in
> > effect dropping the "class" moniker from the name. It would have to be
> > larger than it is currently, of course.
> >
> > Cons:
> > - we would have to abandon the notion of "limitless metaspace" - but if
> we
> > run with class space this has never been true anyway since the number of
> > classes is limited by the size of the compressed class space.
> > - virtual process size would go up since it now needs to be as large as
> the
> > total projected metaspace size.
> > - we may need to expand narrow Klass pointer encoding to include
> shifting,
> > if 4G are not enough to hold all metaspace data.
> >
> > Pros:
> > - this would simplify a lot of code.
> > - this would save real (committed) memory, since we save quite a bit of
> > overhead.
> > - we could narrow-pointer-encode other metadata too, not only Klass
> > pointers, should that ever be interesting
> >
> > ... but that is not part of this JEP.
> >
> > ---
> >
> > With this patch, we have a much stronger separation of concerns, and it
> > should be easy to reuse the metaspace allocator in other hotspot areas as
> > well. For instance, ResourceAreas and friends could be replaced by
> > MetaspaceArenas. They do almost the same thing. And as we have seen in
> the
> > past at SAP, the C-heap backed hotspot Arenas can be a pain since spikes
> in
> > Arena usage lingers forever as process footprint (we even once rewrote
> > parts of the arena code to use mmap, which is just on a more primitive
> > level what Metaspace does).
> >
> > ---
> >
> > I know this is short notice, but there will be a call for interested
> people
> > tomorrow at 11AM ET. In case any potential Reviewers are interested just
> > drop me a short note.
> >
> > ---
> >
> > Thank you, Thomas
> >
> >
> > [1]
> >
> http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/guide/reduction-small-loaders.pdf
> > [2]
> >
> http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/guide/review-guide.pdf
> > [3]
> >
> http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/guide/review-guide.html
> > [4]
> >
> http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/webrev-all/webrev/
> > [5]
> >
> http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/webrev-core/webrev/
> > [6]
> >
> http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/webrev-test/webrev/
> > [7]
> >
> http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/webrev-misc/webrev/
> > [8] https://bugs.openjdk.java.net/browse/JDK-8251342
> > [9] https://bugs.openjdk.java.net/browse/JDK-8251392
>
>


More information about the hotspot-gc-dev mailing list