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

Tue Oct 6 06:48:51 UTC 2020

On Mon, 5 Oct 2020 13:38:03 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi all,
>> 
>> this is the continuation of the ongoing review for the JEP387 implementation (last rounds see [1] [2]). Sorry for the
>> delay, had vacation then the entrance of Skara delayed things a bit.
>> For the delta diff please see [3].
>> 
>> This is the first time I do a large PR after Skara, so if something is wrong please bear with me. I cannot answer all
>> feedback individually in this PR body, but I incorporated almost all into the new revision.
>> What changed since the last version:
>> 
>> - I renamed most metaspace files back to the original naming scheme or to something similar, hopefully capturing the
>>   group consent.
>> 
>> - I changed the way allocation guards are checked if MetaspaceGuardAllocations is enabled. Before, I would test for
>>   overwrites upon CLD destruction, but since that check was subject to VerifyMetaspaceInterval it only ran for every nth
>>   class loader which made it rather pointless. Now I run it always.
>> 
>> - I also improved the printout on block corruption, and log block corruption unconditionally before asserting.
>> 
>> - I also fixed up and commented the death test which tests for allocation overwriters (test_allocationGuard.cpp)
>> 
>> Side note, I find the corruption check very useful but if you guys think it is too much I still can remove the feature.
>> 
>> - In ChunkManager::purge() I improved the comments after discussions with Leo.
>> 
>> - I fixed a bug with VerifyMetaspaceInterval: if set to 1 the "SOMETIMES" sections were supposed to fire always, but due
>>   to a one-off error they only fired every second time. Now, if -XX:VerifyMetaspaceInterval=1, the checks really run
>>   every time.
>> 
>> - Fixed indentation issues as Leo requested
>> 
>> - Rewrote the condition and the assert in VirtualSpaceList::allocate_root_chunk() as Leo requested
>> 
>> - I removed the "can_purge" logic from VirtualSpaceList. The list does not need to know. It just should iterate all nodes
>>   and attempt purging, and if a node does not own its ReservedSpace, it refuses to be purged. That is simpler and more
>>   flexible since it allows us to have list with purge-able and non-purge-able nodes.
>> 
>> - and various smaller fixes, mainly on request of Leo.
>> 
>> @lkorinth:
>> 
>>> VirtualSpaceNode.hpp
>>>
>>>102   // Start pointer of the area.
>>>103   MetaWord* const _base;
>>>
>>>How does this differ from _rs._base? Really needed?
>>>
>>>105   // Size, in words, of the whole node
>>>106   const size_t _word_size;
>>>
>>>Can we not calculate this from _rs.size()?
>> 
>> You are right, _base and _word_size are directly related to the underlying space. But I'd prefer to leave it the way it
>> is. Mainly because ReservedSpace::_base and ::_size are nonconst and theoretically can change under me. It is highly
>> improbable but I'd like to know. Note that VirtualSpaceNode::verify checks that.  Should we clean up ReservedSpace at
>> some point and make those members const - as they should be - then I would rewrite this as you suggest.
>> Thanks, again, for all your review work!
>> 
>> ------
>> 
>> 
>> [1] https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041162.html
>> [2] https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-September/041628.html
>> [3] https://github.com/openjdk/jdk/commit/731f795bc0c1c502dc6cac8f866ff45a15bdd02d
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review Feedback Richard 1

Hi Thomas,

this is batch 2. I'm now at 61/166 files :)

Cheers, Richard.

src/hotspot/share/memory/metaspace/binList.hpp line 135:

> 133:         i2++;
> 134:         m >>= 1;
> 135:       }

I think you overlooked my proposal to make use of `count_trailing_zeros()` instead of this while-loop.

src/hotspot/share/memory/metaspace/metaspaceArena.cpp line 81:

> 79:            METACHUNK_FULL_FORMAT_ARGS(c));
> 80:   }
> 81: }

Have you thought about splitting the chunk if possible and returning the free chunk to the ChunkManager? If you return
it to the free list of this class loader then only this loader can make use of it. If you return it to the CM then
every loader can make use of it and, potentially, it can even be uncommitted.

src/hotspot/share/memory/metaspace/counters.hpp line 108:

> 106:     assert(old >= 1,
> 107:         "underflow (" UINT64_FORMAT "-1)", (uint64_t)old);
> 108: #endif

The assertion is incorrect because it is not atomic wrt the actual update. I commented on this before. It should be
fairly easy to write a multi-thread gtest that produces false positives of the assertion (if it is easy to write a
multi-threaded gtest). The person affected by a false positive crash will be not too amused. I suggest to fix or remove
the assertion.

src/hotspot/share/memory/metaspace/counters.hpp line 121:

> 119:     assert(old >= v,
> 120:         "underflow (" UINT64_FORMAT "+" UINT64_FORMAT ")", (uint64_t)old, (uint64_t)v);
> 121: #endif

The assertion is incorrect because it is not atomic wrt the actual update (see above). I suggest to fix or remove the
assertion.

src/hotspot/share/memory/metaspace/metachunk.cpp line 141:

> 139: void Metachunk::uncommit() {
> 140:   MutexLocker cl(MetaspaceExpand_lock, Mutex::_no_safepoint_check_flag);
> 141:   uncommit_locked();

`uncommit()` seems to be unused.

src/hotspot/share/memory/metaspace/metachunk.hpp line 253:

> 251:   // commit granule size (in other words, we cannot uncommit chunks smaller than
> 252:   // a commit granule size).
> 253:   void uncommit();

`uncommit()` seems to be unused.

src/hotspot/share/memory/metaspace/metaspaceArena.hpp line 114:

> 112:
> 113:   // free block list
> 114:   FreeBlocks* fbl() const                       { return _fbl; }

`fbl()` seems to be unused.

src/hotspot/share/memory/metaspace/chunkManager.cpp line 349:

> 347:         c->uncommit_locked();
> 348:       }
> 349:     }

This seems redundant. To me it looks as if all chunks larger than `commit_granule_words()` are uncommited anyway when
they are returned to the free list. What am I missing?

Oh wait, I found a mail where you explained:

> Arguably, (2) may be redundant since we uncommit chunks >= commit granule size
> already before, when returning them to the ChunkManager (see
> Chunkmanager::return_chunk_locked()). I left it in as a safeguard.

You should explain this in the comment. I'd actually prefer only to assert that
all blocks on the free list are uncommitted.

src/hotspot/share/memory/metaspace/metaspaceReporter.cpp line 292:

> 290:     non_class_cm_stat.print_on(out, scale);
> 291:     out->cr();
> 292:   }

Unconditional line 272 and conditional lines 274, and 289 are identical. Looks like a bug (double accounting).

src/hotspot/share/memory/metaspace/chunkManager.hpp line 102:

> 100:
> 101:   // Uncommit all chunks equal or below the given level.
> 102:   void uncommit_free_chunks(chunklevel_t max_level);

`uncommit_free_chunks(chunklevel_t max_level)` is declared but not defined. Should be removed.

src/hotspot/share/memory/metaspace/chunklevel.hpp line 39:

> 37: // Chunks are managed by a binary buddy allocator.
> 38:
> 39: // Chunk sizes range from 1K to 4MB (64bit).

Also on 32 bit, right? I don't see a distinction.

src/hotspot/share/memory/metaspace/commitMask.hpp line 123:

> 121:   // Mark a whole address range [start, end) as committed.
> 122:   // Return the number of words which had already been committed before this operation.
> 123:   size_t mark_range_as_committed(const MetaWord* start, size_t word_size) {

The return value is not used. The statements to compute it should be removed.

src/hotspot/share/memory/metaspace/commitMask.hpp line 139:

> 137:   // Mark a whole address range [start, end) as uncommitted.
> 138:   // Return the number of words which had already been uncommitted before this operation.
> 139:   size_t mark_range_as_uncommitted(const MetaWord* start, size_t word_size) {

The return value is not used. The statements to compute it should be removed.

src/hotspot/share/memory/metaspace/freeBlocks.hpp line 42:

> 40: // Class FreeBlocks manages deallocated blocks in Metaspace.
> 41: //
> 42: // In Metaspace, allocated memory blocks may be release prematurely. This is

Typo: release

src/hotspot/share/memory/metaspace/freeChunkList.hpp line 69:

> 67: //    uncommitted or committed to at least one commit granule size).
> 68: //  - in all likelihood chunks, when added to this list, are either fully committed
> 69: //    or fully uncommitted (see Settings::uncommit_on_return_min_word_size()).

`Settings::uncommit_on_return_min_word_size()` does not exist.

src/hotspot/share/memory/metaspace/freeChunkList.hpp line 179:

> 177:     return NULL;
> 178:   }
> 179:

Seems to be unused.

src/hotspot/share/memory/metaspace/freeChunkList.hpp line 63:

> 61: //  n committed words to satisfy the caller requested committed word size. We stop
> 62: //  searching at the first fully uncommitted chunk.
> 63: //

The comment is outdated, isn't it?

src/hotspot/share/memory/metaspace/metachunk.hpp line 154:

> 152:   // This means only read or modify these members under expand lock protection.
> 153:   Metachunk* _prev_in_vs;
> 154:   Metachunk* _next_in_vs;

I think the field `_next_in_vs` could be eliminated. The next chunk begins at the `end()` of this chunk.

src/hotspot/share/memory/metaspace/metachunk.hpp line 243:

> 241:
> 242:   bool ensure_fully_committed()           { return ensure_committed(word_size()); }
> 243:   bool ensure_fully_committed_locked()    { return ensure_committed_locked(word_size()); }

`ensure_fully_committed()` and `ensure_fully_committed_locked()` seem to be unused.

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

Changes requested by rrich (Committer).

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