RFR: 8312132: Add tracking of multiple address spaces in NMT [v105]

[Johan Sjölen]

On Thu, 23 May 2024 08:26:13 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> test/hotspot/gtest/nmt/test_vmatree.cpp line 156:
>> 
>>> 154:       }
>>> 155:       i++;
>>> 156:     });
>> 
>> This doesn't really test the state, nor the stack. It also seems to be a lot of code for a single-use test. Similar in other tests.
>> 
>> ---
>> 
>> Proposal: since you have the recurring pattern of doing something with the tree, then checking its expected state, write a check function in the fixture (e.g. assert_tree_state() ) that does that.
>> 
>> Then use it like this:
>> 
>> // Committing in middle of reservation ends with a sequence of 4 nodes
>> TEST_VM_F(VMATreeTest, commit_in_middle_of_reservation) {
>> 
>> 	... reserve, commit in middle, then
>> 
>> 	const AddressState expected_states[] = {
>> 		{ 0,  .... },
>> 		{ 25,  .... },
>> 		{ 50,  .... },
>> 		{ 100,  .... } };
>> 	assert_tree_state(expected_states, 4);
>> }
>> 
>> 
>> And to preserve your sanity when constructing AddressState with its many components, you can add a helper that allows that in a human-friendly form.
>> 
>> For example:
>> A string that encodes flag, stack, and reserved/Commit state in three letters. 
>> First letter: A-H, let this be one of 8 selected MEMFLAGS or - for mtNone
>> Second letter: a-d, let this be one of 4 selected stacks, or - for no stack/empty
>> Third letter: R or C , resreved, committed or none, or - for unreserved
>> Season to taste.
>> 
>> Then, e.g. for reserving 0..100, and committing 25..50, you write:
>> 
>> 
>> const AddressState expected_states[] = {
>> 	makestate( 0, "---", "AaR"),
>> 	makestate(25, "AaR", "AaC"),
>> 	makestate(50, "AaC", "AaR"),
>> 	makestate( 0, "AaR", "---") };
>> assert_tree_state(expected_states, 4);
>> 
>> 
>> Now you can write a bunch of corner-case tests without being too verbose, intent is immediately clear. As an added bonus, it checks for tree state *exactly*, e.g. which nodes are part of the tree, which are not, in which order, etc.
>> 
>> If you reuse that string format for reserving etc, you could do this:
>> 
>> 
>>     reserve_or_commit(0, 100, "AaR");
>>     reserve_or_commit(25, 50, "AaC");
>> 
>> 
>> or just 
>> 
>> 
>>     do(0, 100, "AaR");
>>     do(25, 50, "AaC");
>> 
>> 
>> From there one, one could even auto-build the expected state, but if too much automatism goes into the test, this increases the possibility for errors creeping in one never finds because they make the test go green.
>
> While going for a walk after writing this, I realized an alternative to a string would be just to define those AddressState directly as global constants.
> 
> static const AddressState AaR = ....
> 
> With e.g. 4 flags, 2 stacks and 3 states, this would come to 24 states. With a macro, this could even be easier.
> 
> Up to you.
> 
> If you put those into an array, you can later down in the random-tester-function just chose a state randomly from that array by random index. You don't have to roll the dice three times to select flag, stack and state.

I'm going to take some time to digest these ideas. I'm not a fan of the string-based approach, I much prefer longer but more obvious code. The latter approach might work out. Still, a general tool is to me not preferable. The typical case is probably not that you need to read every test case, but a specific one which fails. Having some repetition in other tests shouldn't bother you then, but having to jump into a generalized DSL for state assertions do.

>This doesn't really test the state, nor the stack. It also seems to be a lot of code for a single-use test. Similar in other tests.

Sorry, I don't understand what you mean by this.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612015180