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

[Thomas Stuefe]

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

>>> @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.

Hey @jdksjolen, hier is a thougt. Maybe for this RFE, maybe for future rework:

We now add a function set_tag to modify the tag across a range of regions. That requires us to preserve the rest of the properties. So, you essentially go through the sections, read old property set, modify this one property, then exchange the old property set for this section with the new property set. The tree does the rest automagically (coalescing, splitting etc).

But it occurred to me, that we may need this functionality for other properties, too. For example, State: We may want to implement "uncommit" as "uncommit across possibly multiple sections that may or may not have been committed, but preserve tags and stacks".

So, we have a general need for "change properties across a range of positions" but where we don't just wipe out the old properties but modify them. So we need to know the old set.

How about we implement this directly in the treap in a more general purpose way. For example:

`void transform(Transformer x, positon from, position to)`

with Transformer being a functor that gets the old property set (and possibly the positions of the old range, though that may not even be needed) and returns a new set of properties. `transform` would call the `Transformer` for every section it encounters.

The VMATree transformer for VMATree::set_tag would just return the input properties, but with Tag replaced by the new tag.

A VMATree transformer for a possible "commit" function would return the input properties, but with State=Comitted. So, we could commit over a range of existing sections with different tags or stacks, and that would preserve the stacks and tags.

And so forth. What do you think?

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

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