RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

[Coleen Phillimore]

Sorry I dropped off hotspot-gc in my first review.

A few replies below.

On 8/19/20 3:43 AM, Thomas Stüfe wrote:
> Hi Coleen,
>
> On Wed, Aug 19, 2020 at 12:19 AM Coleen Phillimore 
> <coleen.phillimore at oracle.com <mailto: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.

Yes, _blocks is good.

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

I agree with this.
>
>     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.

This seems like something useful to explore later.

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

Hopefully, there will be more comments from others waiting for you when 
you get back.
Thanks,
Coleen
>
> Cheers, Thomas