RFR: 8251158: Implementation of JEP 387: Elastic Metaspace
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Aug 24 17:18:42 UTC 2020
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
>
More information about the hotspot-runtime-dev
mailing list