RFR: JDK-8302820: Remove costs for NMTPreInit when NMT is off [v4]

[Andrew Dinn]

On Thu, 2 Mar 2023 07:22:49 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> NMTPreInit has been brought into question lately (see [JDK-8299196](https://bugs.openjdk.org/browse/JDK-8299196) and [JDK-8301811](https://bugs.openjdk.org/browse/JDK-8301811)). The points of contention were costs paid when NMT is off, and complexity.
>> 
>> I believe NMTPreInit is vital (for reasons why see discussion under [8299196](https://bugs.openjdk.org/browse/JDK-8299196) ), and removing it would be a severe mistake. So let's address the cost problem.
>> 
>> NMTPreInit, in its current form, incurs costs post-init for lookup table lookup to identify pre-init allocations. Granted, this cost is already pretty low since the load factor of that table is small. But we can avoid that lookup completely by allocating pre-init blocks without malloc headers.
>> 
>> That has two advantages:
>> - costs for NMTPreInit for NMT=off are practically nil now: all that remains is querying NMT tracking level to see if we are pre- or post-init, and we need to do that anyway to see if NMT is switched on. That cost is not going away unless we get rid of NMT itself.
>> - We can delete the lookup table if NMT is off, since we don't need it nomore, and regain 63352 bytes of memory.
>> 
>> -----
>> 
>> I have done my best to come up with a good compromise between complexity, startup speed, and memory consumption. With a bit more complexity, penalties to startup speed could be even more reduced (e.g. by shepherding preallocation headers into their arena). 
>> 
>> But I'm between a rock and a hard place here: more complexity increases the chance of "its too complex, let's just remove it", which is a tiny bit stressful tbh. And the one point I feel strongly about is that getting rid of NMTPreInit would be a grave mistake. I also don't think this code needs more optimization.
>> 
>> (Please note that I enter vacation and won't be able to react promptly to reviews.)
>> 
>> ---
>> 
>> Tests: 
>> - Manually tested linux x64 and x86 (gtests with all NMT permutations; runtime/NMT)
>> - GHAs ongoing
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge branch 'master' into make-nmt-preinit-cheaper-for-nmt-off
>  - Delete LU entries after VM init; fix test for NMT=off case
>  - feedback johan
>  - make-nmt-preinit-cheaper-for-nmt-off

Looks good to go.

Hi Thomas,

I fully agree with the arguments you provided in https://bugs.openjdk.org/browse/JDK-8299196 for retaining NMT pre init tracking. So, I think this change is a necessary improvement.

However, I also agree with Justin that the cost of the leaks will be a case where the sum is greater than the parts due to fragmentation. It may well hurt more than is apparent. So, one of the three options you suggested to clear the redundant table entries should be implemented. I'd be happy to see that as a follow-up patch since:

  - this code is ready to ship and improves the status quo
  - a follow-up patch can investigate which is the best of the 3 options

No nits as far as the code is concerned.

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

Marked as reviewed by adinn (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12642