RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

Thu Aug 20 00:35:37 UTC 2020

Hi Thomas,

Here are some comments from reading through the gtests.  One you already 
answered.

=== Global test comment:

  * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights 
reserved.

These should probably only have 2020 for the Oracle copyright since 
these are new files.

=== metaspace_testhelper.hpp/cpp and metaspaceTestsContexts.hpp

metaspace_testhelper.hpp/cpp is unused.  Looks like it was replaced with 
metaspaceTestContexts.hpp but left
in the changeset. Ok, you said that already.

The include guards weren't updated with the name change:
#ifndef GTEST_METASPACE_METASPACE_TESTHELPER_HPP
#define GTEST_METASPACE_METASPACE_TESTHELPER_HPP

=== metaspaceTestsCommon.hpp

Why does it #include and use everything?

The functions (get_random_size .. check_marked_range) appear to be in 
the global scope, which might affect
linking libjvm.so for gtest in the future.  Could they be added to 
metaspaceTestContext?
Or put in a namespace metaspace ?

=== test_chunkManager.cpp/test_chunkManager_stress.cpp

Can these be combined? They are mostly the same.

=== test_is_metaspace_obj.cpp

// Test the cheerful multitude of metaspace-contains-functions.
Thank you for this!

=== test_metachunk.cpp

You even have ascii art in the test.  This is great.

 >   // start each split test with a fresh MetaspaceTestHelper.

Should be ChunkTestsContext.

=== test_metaspacearena.cpp

 > // TODO: this class is very similar to MetaspaceArenaTestBed in 
test_metaspacearena_stress.cpp.
 > // should be unified.

These tests have a lot of diffs with each other, and don't look the same 
to me. Maybe this is an out-of-date comment
to be removed?

=== test_metaspace_counters.cpp

This seems kind of obvious.  It looks like you wanted it to do more, 
because of the extra includes.
I think you should remove it and add a new test with 
https://bugs.openjdk.java.net/browse/JDK-8252014.

=== test_report.cpp

Why not test this with a jtreg test so you can search the output for 
useful strings. This test doesn't seem useful.

That's all for gtests.  They seem quite thorough, so it's unlikely for 
anyone to accidentally break metaspace.
Thanks,
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