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

[Johan Sjölen]

On Fri, 8 Nov 2024 09:44:34 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Mac OSX failure unrelated to PR.
>
>> @jdksjolen " Last test didn't actually make any sense Since the VMATree will remove the released range when it recognises that it is a no-op"
>> 
>> not so hasty :) The test was fine. You said that set_tag just ignores Released sections in range. So, what should have happened is that in the test, the remaining reserved section should have remained and changed to mtGC. The tree then should consist of `[0, 50) Released, mtNone, nostack) [50, 75) mtGC, si) [75, 100) Released, mtNone, nostack)`.
>> 
>> In other words, the test would test that calling set_tag across a range that contains Released sections does not change said sections.
> 
> I actually am stuck thinking about this, and it's worse than that :-).
> 
> Consider a simpler test:
> 
> ```c++
> {
>     VMATree tree;
>     Tree::RegionData class_shared(si, mtClassShared);
>     tree.reserve_mapping(10, 10, class_shared);
>     tree.set_tag(0, 100, mtCompiler);
> }
> 
> 
> This will crash on an assert. After the `reserve_mapping` the state of the tree is `Res,10 - Res,20`. That is: Two reserved nodes, at position 10 and 20.
> 
> So, when we run `find_enclosing_range(0)`, we get a non-existent range back, because no range encloses `0`. I'm pretty sure that we can have better behavior by interpreting the piece-wise results of `find_enclosing_range`.

> > > @jdksjolen " Last test didn't actually make any sense Since the VMATree will remove the released range when it recognises that it is a no-op"
> > > not so hasty :) The test was fine. You said that set_tag just ignores Released sections in range. So, what should have happened is that in the test, the remaining reserved section should have remained and changed to mtGC. The tree then should consist of `[0, 50) Released, mtNone, nostack) [50, 75) mtGC, si) [75, 100) Released, mtNone, nostack)`.
> > > In other words, the test would test that calling set_tag across a range that contains Released sections does not change said sections.
> > 
> > 
> > I actually am stuck thinking about this, and it's worse than that :-).
> > Consider a simpler test:
> > ```c++
> > {
> >     VMATree tree;
> >     Tree::RegionData class_shared(si, mtClassShared);
> >     tree.reserve_mapping(10, 10, class_shared);
> >     tree.set_tag(0, 100, mtCompiler);
> > }
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > This will crash on an assert. After the `reserve_mapping` the state of the tree is `Res,10 - Res,20`. That is: Two reserved nodes, at position 10 and 20.
> > So, when we run `find_enclosing_range(0)`, we get a non-existent range back, because no range encloses `0`. I'm pretty sure that we can have better behavior by interpreting the piece-wise results of `find_enclosing_range`.
> 
> I wonder:
> 
> The answer could be to start the tree out with a single range, State=Released, mtNone, nostack, from [0 .. max). In other words, have two nodes that border the very start and end of the `position` value range.
> 
> The only problematic part is that one must encode the logic that there cannot be a node preceding the first node - so it has no input state (or it can have one, but should be ignored).
> 
> What do you think? Elegant or too overengineered?

You could alter the `VMATree` to look like that. I'd rather have fewer assumptions made by `VMATree`, and a more complicated `set_tag`. The function can deal with these challenges, no problem.

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

PR Comment: https://git.openjdk.org/jdk/pull/20994#issuecomment-2464652276