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

[Thomas Stuefe]

On Mon, 29 Jul 2024 12:45:46 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Asserting may make sense on debug builds, as you might be making a mistake, but we should have pre-defined behavior for production builds anyhow, no `ShouldNotReachHere()`. I think that valid behavior is to set the flag to whichever was provided in the call.
>> 
>> 
>> Another option is to have a more elaborate heuristic. If you uncommit `[a, b)` then find the first range which starts inside of `[a, b)` and take its memory flag, if none is found then give up and take `mtNone`.
>> 
>> The nicest behavior is an `O(n)` algorithm where we go through all regions in `[a, b)` and convert each to its reserved version with the flag, then uncommit with holes in the range would leave those holes unreserved. 
>> 
>>>Are there valid use cases for reserving ranges with mtNone as a valid NMT flag?
>> 
>> Only in the context of then later setting the memory flag, which I think is a pattern which should be avoided if possible.
>
> I'm only considering the usage of this flag as something which should be used by an `uncommit_region` API, if that wasn't clear.

> Asserting may make sense on debug builds, as you might be making a mistake, but we should have pre-defined behavior for production builds anyhow, no ShouldNotReachHere(). I think that valid behavior is to set the flag to whichever was provided in the call.

I disagree :)

The point of this default mechanism is to take cognitive load from the programmer. I don't want to have to think about the flag when I rely on this mechanism. Having this mechanism, but then still forcing the caller to specify a flag "just in case you commit into unreserved territory" is not a big help.

By using copy_flag=true, I declare that I expect there to be a prior mapping in that range. That is part of the API contract. If that expectation is broken, I did something wrong. This is not something that can happen due to random runtime conditions. Therefore, like any other invalid state it should be handled with an assert. I don't think it makes a lot of sense to think about what to do in release builds: this is an error like many others, and we have many asserts in the JVM and we only rarely encode a valid non-assert path.

> Another option is to have a more elaborate heuristic. If you uncommit [a, b) then find the first range which starts inside of [a, b) and take its memory flag, if none is found then give up and take mtNone.

This does not sound very appealing tbh. In most cases, this would be an error. It is not NMTs responsibility to catch this error, but I would not go out of my way to accomodate it case either. 

> The nicest behavior is an O(n) algorithm where we go through all regions in [a, b) and convert each to its reserved version with the flag, then uncommit with holes in the range would leave those holes unreserved.

You lost me here. This is what we do right now, isn't it? VMATree just replaces all existing mappings between [a, b) with the new mapping?

> I'm only considering the usage of this flag as something which should be used by an uncommit_region API, if that wasn't clear.

Restricting this to uncommit would not use its full potential.

Consider this example: In the Metaspace allocator I would like to commit memory, but that memory can be tagged with either mtClassSpace or mtMetaspace, and in the course of Lilliput we may see even more tags. In all cases I reuse the same allocator code. Here, why should I be forced to have to specify the tag on commit? I already marked the underlying memory with the correct tag when reserving it.

The ability of painting regions with a certain flag and then having subsequent memory operations on that memory preserve the flag is powerful. You can, for example, in Metaspace account memory at the chunk level. Without having to add code everywhere that keeps track of those flags. You only have to paint the chunks you give to a Metaspace arena with the correct flag, and from then on the whole process is automatic.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20330#discussion_r1696340042