RFR: JDK-8256725: Metaspace: better blocktree and binlist asserts [v3]

[Leo Korinth]

On Sat, 21 Nov 2020 09:31:30 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi,
>> 
>> may I have reviews please for this small change. 
>> 
>> To analyze JDK-8256572, I'd like to see more information in asserts for binlist and blocktree.
>> 
>> This patch:
>> 
>> - beefs up assertion messages when verifying binlist and blocktree
>> - adds a canary to the blocktree node to detect overwriters
>> - improves the blocktree printing
>> - adds a gtest death test to test the overwrite detection.
>> 
>> Thanks!
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Quick node checks in blocktree on insert/removal

Looks good. I like the canary in Node. I have three minor comments you should take a look at.

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

> 160:              b->_word_size == real_word_size,
> 161:              "bad block size in list[%u] (" BLOCK_FORMAT ")", index, BLOCK_FORMAT_ARGS(b));
> 162:       MetaWord* const p = (MetaWord*)b;

I suggest not creating a `p` variable and just casting the `b` variable at return. After your change, they have the same value and just differs in type.

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

> 187:       for (Block* b = _blocks[i]; b != NULL; b = b->_next, pos++) {
> 188:         assert(b->_word_size == s,
> 189:                "bad block size in list[%u] at pos %d (" BLOCK_FORMAT ")",

Both `i` and `pos` are signed integers, why not print them as such. I am a little surprised that the compiler did not catch it, so maybe I am missing something.

src/hotspot/share/memory/metaspace/blockTree.cpp line 126:

> 124:       // check size and ordering
> 125:       tree_assert_invalid_node(n->_word_size >= MinWordSize &&
> 126:                                n->_word_size <= chunklevel::MAX_CHUNK_WORD_SIZE, n);

I prefer one assert per line (that is, not using `&&` in asserts). In this case it does not matter as much, as we can deduce which sub-expression failed from the tree printout,  but I think it is a better style, because in _general_ you do not want such an assert to fail.

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

Changes requested by lkorinth (Committer).

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