RFR: JDK-8256725: Metaspace: better blocktree and binlist asserts

Aleksey Shipilev shade at openjdk.java.net
Fri Nov 20 10:34:07 UTC 2020


On Fri, 20 Nov 2020 08:34:18 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!

Stylistic drive-by comments.

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

> 187:       const size_t s = MinWordSize + i;
> 188:       int pos = 0;
> 189:       for (Block* b = _blocks[i]; b != NULL; b = b->_next, pos ++) {

`pos++` seems to be more in-style here.

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

> 42:   ": canary: " INTPTR_FORMAT \
> 43:   ", parent " PTR_FORMAT \
> 44:   ", left " PTR_FORMAT \

So, is this message with ":" (like `canary:`), or without it (like all other fields)?

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

> 53:   p2i((n) ? (n)->_left : NULL), \
> 54:   p2i((n) ? (n)->_right : NULL), \
> 55:   p2i((n) ? (n)->_next : 0), \

This is still pointer, right, judging by `p2i`? So it should return `NULL` in the ternary op?

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

> 61: 
> 62: // This assert prints the tree, then stops (generic message)
> 63: #define assrt0(cond, format, ...) \

The names get hard to read. I thought it was as typo initially :) Suggestion: `tree_assert`?

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

> 87:   // assert a condition with information about node "n"
> 88:   #define assrt0n(cond, failure_node) assrt0(cond, "Invalid node: " NODE_FORMAT, NODE_FORMAT_ARGS(failure_node))
> 89: 

Do you think this assert is only usable in this method? I would probably be cleaner to move it next to the assertion it chains with.

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

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


More information about the hotspot-runtime-dev mailing list