RFR: 8340103: Add internal set_flag function to VMATree [v16]

[Thomas Stuefe]

On Wed, 6 Nov 2024 10:05:43 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi!
>> 
>> The old VirtualMemoryTracker has a method set_reserved_region_type(address, flag). We implement this for the new VMATree implementation by altering the signature slightly to set_reserved_region_type(address, size, flag). This simplifies the implementation greatly for our new data structure and leads to trivial changes for the callers (all callers already know the size).
>> 
>> This PR implements the internal implementation along with tests, but does not change any callers.
>> 
>> I also do a few cleanups:
>> 
>> - Change `Node` to `TreeNode` in tests, we've seen build failures because of this (probably a precompiled headers issue)
>> - Add a few `print_on` methods for  easy debugging
>> - Add a `size` alias, it was a bit confusing that some functions took an argument `position sz`, so changed that to `size sz` 
>> 
>> Thanks.
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make a comment about this nifty trick

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

> 288: TEST_VM_F(NMTVMATreeTest, SetTag) {
> 289:   using State = VMATree::StateType;
> 290:   auto i = [](MemTag f) -> uint8_t { return (uint8_t)f; };

This does not seem to be used anywhere (I can comment it out, code still builds).

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

> 304:     for (int i = 0; i < len; i++) {
> 305:       testrange expect = expected[i];
> 306:       VMATree::VMATreap::Range found = tree.tree().find_enclosing_range(expect.from);

Here, you don't test for sequence: the code relies on expected to be ordered. Just to be sure, please add an assert (lowercase) here that the current range from is == the last range to.

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

> 317:       EXPECT_EQ(expect.stack, found.end->val().in.stack());
> 318:     }
> 319:     EXPECT_EQ(len+1, tree.tree().size());

Ah, this is the "and only those" part. We iterate, then compare the number of items in expected.

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

> 417:   }
> 418: }
> 419: 

Do we allow address holes? If yes, please add a test where we set_tag over an address hole. If the expected behavior is to ignore the hole, this should be easily tested.

Otherwise, if we assert in case of set_tag over a hole, its fine. We could write a death test to test the assert, but I don't think that is worth the test cycles (death test are expensive).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20994#discussion_r1830742461
PR Review Comment: https://git.openjdk.org/jdk/pull/20994#discussion_r1830749272
PR Review Comment: https://git.openjdk.org/jdk/pull/20994#discussion_r1830751028
PR Review Comment: https://git.openjdk.org/jdk/pull/20994#discussion_r1830759888