RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

Thu Aug 20 13:46:39 UTC 2020

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