On Wed, 4 Mar 2026 14:03:57 GMT, Afshin Zafari <[email protected]> wrote:
>Thank you for bringing this change. Would you please explain what we gain by >reordering the calls and moving release_memory out of the lock area? >Performance gain? Why and how much? I created a (pretty contrived) benchmark with 16 threads reserving/releasing memory 2000 times and saw a ~7.5% speedup with these changes. However, in reality I don't think we expect many concurrent virtual memory operations, so NMT lock contention is unlikely to matter for most workloads. The reason I proposed this PR is mainly because it's a fairly non-invasive opportunity to shrink lock scope, and a shorter critical section is generally better than a longer one. Where possible, it's also probably nicer to have the NMT lock only be responsible for protecting NMT operations, not the OS calls too. > Also, if NMT records the release amount and pd_release_memory() fails, the > amount reported by NMT at exit would be wrong since the released size is > already de-accounted. That is a good point. I had thought that failing fatally upon `pd_release_memory` would prevent us from needing to update NMT, but I hadn't accounted for the NMT report created at exit. `pd_release_memory` only fails if bad arguments are provided or on some catastrophic system failure. - In the case of catastrophic system failure in `pd_release_memory`, updating NMT prematurely probably doesn't matter because it's hard to know what state the memory is actually in now anyway. Even if we hadn't updated NMT prematurely, the NMT accounting may still now be inaccurate. - In the case of bad arguments causing a crash, an NMT report containing the region we are trying to release could be helpful in diagnosing the problem. In such cases, we would need to do something like patch the NMT accounting. That would be more work than it's worth since we'd need to retrieve the orignal memtag and NativeCallStack. I'll think about this a bit more, and if there's no good solution, I'll close the issue. ------------- PR Comment: https://git.openjdk.org/jdk/pull/29962#issuecomment-3999367774
