RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

[Coleen Phillimore]

Looking though more code:

=== metaspaceArena.cpp

bool MetaspaceArena::attempt_enlarge_current_chunk(size_t 
requested_word_size) {

Since this function has most lines starting with c->, it seems like this 
function should be in metachunk

current_chunk()->attempt_enlarge_current_chunk(requested_size, 
next_chunk_level());

Coleen

On 8/19/20 8:31 PM, Coleen Phillimore wrote:
>
> 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
>