RFR: 8335091: NMT: VMATree reserve_mapping and commit_mapping APIs need MEMFLAGS while un/-committing API has no MEMFLAGS arg [v4]

[Gerard Ziemski]

On Tue, 30 Jul 2024 12:25:04 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> In committing a region, it is not mandatory to provide a MEMFLAGS flag where the committed region inherits the flag from the main region it resides in.
>> In un-committing there is no need to a  MEMFLAGS at all. 
>> The `register_mapping` API of the VMATree *requires* a MEMFLAGS (via metadata arg) in both of these two operations. To do the flag inheriting, it is possible to copy the flag of the left node in the tree to the newly inserted ones. 
>> 
>> An optional bool arg (default is false) is added to VMATree API to copy the existing flag of the left node to the new nodes.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   invalid cases of register_mapping are handled.

Changes requested by gziemski (Committer).

src/hotspot/share/nmt/vmatree.cpp line 107:

> 105:       return false;
> 106:     };
> 107:     VMATree::SummaryDiff invalid_diff(-1);

Can we replace hardcoded `-1` with some a constant in:

`VMATree::SummaryDiff invalid_diff(-1);`

and what is the caller supposed to do with that? How would it check for the special INVALID_DIFF value?

For example, in `src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp` we define:

`constexpr static const StackIndex invalid = std::numeric_limits<StackIndex>::max() - 1;`

and offer the API:

`static bool is_invalid(StackIndex a)`

src/hotspot/share/nmt/vmatree.hpp line 192:

> 190: 
> 191:  public:
> 192:   SummaryDiff reserve_mapping(position from, position sz, const RegionData& metadata) {

In

`SummaryDiff register_mapping(position A, position B, StateType state, const RegionData& metadata, bool copy_flag = false);`

`position B` is clearly a position of its own, however, in

`SummaryDiff reserve_mapping(position from, position sz, const RegionData& metadata)`

`position sz` is **size** or **delta**. We should rename the parameter to reflect that intended functionality better.

I would probably prefer to see us introduce:

`using position_size = size_t;`

and use as appropriate, instead, of forcing everything to be `position`, to help keep the API usage straight.

test/hotspot/gtest/nmt/test_vmatree.cpp line 195:

> 193:       tree.reserve_mapping(10000, 10, rd2);
> 194:       tree.visit_in_order([&](Node* node) {
> 195:         if ((size_t)node->key() == 0    ) { EXPECT_EQ(node->val().out.flag(), mtTest) << "failed at: " << node->key(); }

Is there a way not to need this casting?

`(size_t)node->key()`

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

PR Review: https://git.openjdk.org/jdk/pull/20330#pullrequestreview-2277926269
PR Review Comment: https://git.openjdk.org/jdk/pull/20330#discussion_r1742376211
PR Review Comment: https://git.openjdk.org/jdk/pull/20330#discussion_r1742349227
PR Review Comment: https://git.openjdk.org/jdk/pull/20330#discussion_r1742353510