RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

[Thomas Stüfe]

Hi Coleen,

On Wed, Aug 19, 2020 at 12:19 AM Coleen Phillimore <
coleen.phillimore at oracle.com> wrote:

>
> 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.
>

A bad habit of mine, idly pressing space white thinking. I'll fix those.


>
> === 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.
>

I agree. I kept this class here to keep the patch confined to
memory/metaspace. I would like cleanups like this as follow ups to the main
patch, as you suggested. I created
https://bugs.openjdk.java.net/browse/JDK-8252014 to keep track of this.


> === freeBlocks.hpp
>
> I like how the small blocks and tree of freed blocks are together
> (except see below).
> This has a really good comment.
>

Thanks.


>
> #ifndef SHARE_MEMORY_METASPACE_LEFTOVERBINS_HPP
> #define SHARE_MEMORY_METASPACE_LEFTOVERBINS_HPP
> Wrong ifdef guard names.
> #endif // SHARE_MEMORY_METASPACE_CHUNKMANAGER_HPP
>
>
Ok


> === binList.hpp
>
>    block_t* _v[num_lists];
>
> Some slightly longer name would be better than _v.  Maybe _small_blocks
> or something like that?
>
>
Would _blocks be acceptable? Since the distinguisher 'small' makes no sense
here, it is the only member in the class.


> // 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.
>
>
Ok


> === blockTree.hpp/cpp
>
> I didn't review this code.  Someone suggested using std::map instead but
> we're not using std:: yet.
>
>
I don't really like that idea. Using stl maps for structures as these
in-place blocks seems, depending on how it is done, either risky or
wasteful.

In this tree payload and nodes are identical and the size of the payload is
the key. Like the Linux rbtree, really. STL map keeps these entities
separate: a KV pair holds K (block size) and V (the block). So we spent
additional memory and allocation to keep the pair itself, and K is also
kept redundant (since the block already knows its size). This is if we use
stl map safely-but-naively. I guess one could get stl::map to behave
exactly as the Linux rb tree - taking KV pair allocation into our own hand
and somehow eliminating the K storage redundancy. But I would think we end
up with a solution way less maintainable than what I have suggested. You'd
need intimate knowledge of stl map to make sure we this works, and across
all platforms and all STL implementations. I'd think maintaining this
little tree thing is cheaper.

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.
>
>
Oh good point.

I would not like to hard-code the assumption that class space only contains
Klass though; I dimly remember an RFR with Ioi where he tried storing other
things there as well. IIRC he found some other solution. But my point is
that this assumption is fragile.

But a "soft assumption" would be fine. E.g. creating the BinList on the
fly, only when needed, and separately from the blocktree. Easy to implement
too.

We don't need NMT to analyze. BinList currently has 32 slots, + counter +
mask. We pay 272 bytes per structure, or 34 words. Which we would save per
CLD.

=== 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.
>

I'll fix that.


>
> // 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.
>

Yes, it only happens when testing. The rest is future thoughts, in case we
ever use a metaspace context for other purposes (I am currently
experimenting with using them as Arena replacement).

I will adapt the comment.


> === 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.
>
>
My god how did this get there. How did this ever compile. This is an
accidental paste from some code move I did before posting the review. And
yes, seems both fill_with_pattern and check_pattern are unused. Remnants
from some earlier version. I'll remove both.


>
> === metaspace_test.hpp/cpp
>
> I keep thinking this file is in the wrong place.  Can you rename
> metaspaceTestHelper? or some name like that?
>

Will do. These classes must live somewhere in metaspace since they are used
by whitebox.cpp. I could move them there directly. But I wanted to keep
them separate.


>
> === 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.
>

I agree.


>
> === 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.
>
>
Ok.


>
> === 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.
>
>
I agree, this seems odd. Will move as suggested.


> === 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
>

Thanks for the review, Coleen. I will fix and post an update. I'll have
vacation the rest of the week and will post an updated webrev start of next
week, with your changes and some other small fish I did.

Cheers, Thomas