RFR: 8251158: Implementation of JEP 387: Elastic Metaspace [v2]

[Thomas Stuefe]

On Mon, 28 Sep 2020 14:36:42 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove empty lines from include sections
>
> Besides my comment about globals.hpp option MetaspaceGuardAllocations, my comments are minor things and I approve this
> change.  There might be some additional things we find that we'll want to change once this code is integrated.  This is
> a significant improvement to metaspace memory management.  Great work, @tstuefe !

Hi Leo,

> I just have a few cosmetic comments. Otherwise it looks good to me.
> 
> If you sort by ASCII order, I get a few unordered includes:
> ("." < "/" < "a-z,A-Z")
> ....

I will fix all those.

> 
> blockTree.hpp
> add a space after loop keyword "for(;;) {" -> "for (;;) {"
> 
> blockTree.cpp
> add a space after loop keyword "} while(0)" -> "} while (0)" (twice in file)
> 

Okay

> I still think we should try to get the initializer list indented
> somewhat consistently.

I agree, will do.

> I think this sort of deep verification should be in a gtest instead, at least for blockTree. Note, this can be
> fixed/discussed in a future RFE.

:( This would affect a lot of my code since I do a lot of in-file verifications. Yeah, lets discuss this separately.

> src/hotspot/share/memory/metaspace/classLoaderMetaspace.cpp line 64:
> 
>> 62:   , _space_type(space_type)
>> 63:   , _non_class_space_arena(NULL)
>> 64:   , _class_space_arena(NULL)
> 
> Ok, I see what @lkorinth was commenting on.  In the coding standard, the normal indentation level is 2, but we don't
> specify it for initializers.  Generally it seems what looks good, maybe 4, maybe aligned somewhat with the arguments.
> That said to me it doesn't matter that much, BUT most of the code has the punctuation at the end of the line, not the
> beginning.  I think it looks weird at the beginning.  Can you change these?

Yes, I agree with you two. I will find a common form and fix all my initializations.

> src/hotspot/share/memory/metaspace/commitMask.hpp line 92:
> 
>> 90:     check_pointer(start + word_size - 1);
>> 91:   }
>> 92: #endif
> 
> If these are for asserts, they can be defined in the .cpp file.

Ok

> src/hotspot/share/memory/metaspace/metachunk.hpp line 272:
> 
>> 270:     _vsnode = node; _base = base; _level = lvl;
>> 271:     _used_words = _committed_words = 0; _state = State::Free;
>> 272:     _next = _prev = _next_in_vs = _prev_in_vs = NULL;
> 
> Is this the same as clear() ?

Yes, I'll reuse clear() instead.

> src/hotspot/share/memory/metaspace/metachunkList.hpp line 60:
> 
>> 58:   void add(Metachunk* c) {
>> 59:     // Note: contains is expensive (linear search).
>> 60:     ASSERT_SOMETIMES(contains(c) == false, "Chunk already in this list");
> 
> Can you make this something like:  DEBUG_ONLY(verify_contains();) and hide ASSERT_SOMETIMES in the .cpp file?

Ok

> src/hotspot/share/memory/metaspace/metachunkList.hpp line 29:
> 
>> 27: #define SHARE_MEMORY_METASPACE_METACHUNKLIST_HPP
>> 28:
>> 29: #include "memory/metaspace/counters.hpp"
> 
> Is this header file needed now?

Yes, since we still use IntCounter.

> src/hotspot/share/runtime/globals.hpp line 1589:
> 
>> 1587:                                                                             \
>> 1588:   product(bool, MetaspaceGuardAllocations, false,                           \
>> 1589:           "Metapace allocations are guarded.")                              \
> 
> This should be DIAGNOSTIC or develop() but not product.

Ok!

-------------

PR: https://git.openjdk.java.net/jdk/pull/336