RFR: 8251158: Implementation of JEP 387: Elastic Metaspace
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Aug 20 00:31:39 UTC 2020
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
More information about the hotspot-runtime-dev
mailing list