RFR: 8251158: Implementation of JEP 387: Elastic Metaspace
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Aug 18 22:17:19 UTC 2020
Hi Thomas,
I've read through the new implementation files. This looks really
good! These are my initial comments. There aren't many and nothing of
substance really. Some files need more in-depth review though. I
haven't looked at the tests, which are more difficult. I'll do that
tomorrow.
General comment: Some files have a lot of vertical white space, like
two or more blank lines together: eg. metachunk.cpp at the end. More
about that below.
=== counter.hpp
This seems like a general utility that should be in utilities so that it
can be generalized for use in other places. I didn't review it
carefully to suggest it go there right now, but we should file another
RFE to do that. Kim has some ideas about it.
=== freeBlocks.hpp
I like how the small blocks and tree of freed blocks are together
(except see below).
This has a really good comment.
#ifndef SHARE_MEMORY_METASPACE_LEFTOVERBINS_HPP
#define SHARE_MEMORY_METASPACE_LEFTOVERBINS_HPP
Wrong ifdef guard names.
#endif // SHARE_MEMORY_METASPACE_CHUNKMANAGER_HPP
=== binList.hpp
block_t* _v[num_lists];
Some slightly longer name would be better than _v. Maybe _small_blocks
or something like that?
// These aren't used
typedef BinListImpl<2, 8> BinList8;
typedef BinListImpl<2, 16> BinList16;
typedef BinListImpl<2, 64> BinList64;
This file should be binList.hpp rather than binlist.hpp.
=== blockTree.hpp/cpp
I didn't review this code. Someone suggested using std::map instead but
we're not using std:: yet.
blocktree.hpp/cpp should be blockTree.hpp/cpp
Since for class metaspace, the reclaimed InstanceKlass are generally
bigger than the largest BinList size, maybe class type MetaspaceArenas
should only have a blockTree and not a SmallBlocks, so not the whole
freeList.hpp structure. I think we could do some footprint analysis
with NMT and see if this might help. It's fairly rare unless you have a
lot of redefinition. I don't think this should be changed in this
change though.
=== metaspaceContext.cpp
#define LOGFMT "SpcMgr @" PTR_FORMAT " (%s)"
#define LOGFMT_ARGS p2i(this), this->_name
These don't seem to be used and is SpcMgr a reference to the old
SpaceManager? Correction, I see how it's used but you should change it
to not refer to Space manager.
// Destroys the context: deletes chunkmanager and virtualspacelist.
// If this is a non-expandable context over an existing space, that
space remains
// untouched, otherwise all memory is unmapped.
MetaspaceContext::~MetaspaceContext() {
delete _cm;
delete _vslist;
}
Why would you ever do this? I thought the two contexts were global. If
it's for the tests, add a comment here.
=== metachunk.cpp
class UnsortedMetachunkList : public AbstractMetachunkList {
This class is declared in a strange place (in a function) and appears
unused. check_pattern() appears unused which is good because it's a lot
of code.
=== metaspace_test.hpp/cpp
I keep thinking this file is in the wrong place. Can you rename
metaspaceTestHelper? or some name like that?
=== internStats.hpp
Indentation is inconstent and too much vertical white space at the end.
Should be renamed internalStats.hpp/cpp to match the class name. This
looks like a nice improvement. One of the things that made metaspace
code hard to read was that there was a lot of code for statistics mixed
in everywhere.
=== arenaGrowthPolicy.hpp/cpp
I like this!
=== chunkLevel.hpp/cpp
I'm fine with the nested namespace chunklevel, rather than an AllStatic
class ChunkLevel. The names of the files should probably be changed to
chunklevel.hpp/cpp though.
=== metaspaceEnums.hpp/cpp
This set of files seems like they are misnamed and should be
encorporated into metaspace.hpp, since they are used outside of the
memory/metaspace directory.
=== settings.hpp
Include guards are wrong names.
#ifndef SHARE_MEMORY_METASPACE_CONSTANTS_HPP
#define SHARE_MEMORY_METASPACE_CONSTANTS_HPP
#endif // SHARE_MEMORY_METASPACE_BLOCKFREELIST_HPP
I actually took the allocation path when looking at this. Thank you for
keeping many of the function names because that helped.
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
More information about the hotspot-runtime-dev
mailing list