JDK-8256844: Make NMT late-initializable

[tstuefe]

Short: this patch makes NMT available in custom-launcher scenarios and during gtests. It simplifies NMT initialization. It adds a lot of NMT-specific testing, cleans them up and makes them sideeffect-free.

---

NMT continues to be an extremely useful tool for SAP to tackle memory problems in the JVM.

However, NMT is of limited use due to the following restrictions:

• NMT cannot be used if the hotspot is embedded into a custom launcher unless the launcher actively cooperates. Just creating and invoking the JVM is not enough, it needs to do some steps prior to loading the hotspot. This limitation is not well known (nor, do I believe, documented). Many products don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this problem limits NMT usefulness greatly since our VMs are often embedded into custom launchers and modifying every launcher is impossible.
• Worse, if that custom launcher links the libjvm statically there is just no way to activate NMT at all. This is the reason NMT cannot be used in the gtestlauncher.
• Related to that is that we cannot pass NMT options via JAVA_TOOL_OPTIONS and -XX:Flags=<file>.
• The fact that NMT cannot be used in gtests is really a pity since it would allow us to both test NMT itself more rigorously and check for memory leaks while testing other stuff.

The reason for all this is that NMT initialization happens very early, on the first call to os::malloc(). And those calls happen already during dynamic C++ initialization - a long time before the VM gets around parsing arguments. So, regular VM argument parsing is too late to parse NMT arguments.

The current solution is to pass NMT arguments via a specially prepared environment variable: NMT_LEVEL_<PID>=<NMT arguments>. That environment variable has to be set by the embedding launcher, before it loads the libjvm. Since its name contains the PID, we cannot even set that variable in the shell before starting the launcher.

All that means that every launcher needs to especially parse and process the NMT arguments given at the command line (or via whatever method) and prepare the environment variable. java itself does this. This only works before the libjvm.so is loaded, before its dynamic C++ initialization. For that reason, it does not work if the launcher links statically against the hotspot, since in that case C++ initialization of the launcher and hotspot are folded into one phase with no possibility of executing code beforehand.

And since it bypasses argument handling in the VM, it bypasses a number of argument processing ways, e.g., JAVA_TOOL_OPTIONS.

---

This patch fixes these shortcomings by making NMT late-initializable: it can now be initialized after normal VM argument parsing, like all other parts of the VM. This greatly simplifies NMT initialization and makes it work automagically for every third party launcher, as well as within our gtests.

The glaring problem with late-initializing NMT is the NMT malloc headers. If we rule out just always having them (unacceptable in terms of memory overhead), there is no safe way to determine, in os::free(), if an allocation came from before or after NMT initialization ran, and therefore what to do with its malloc headers. For a more extensive explanation, please see the comment block nmtPreInit.hpp and the discussion with @kimbarrett and @zhengyu123 in the JBS comment section.

The heart of this patch is a new way to track early, pre-NMT-init allocations. These are tracked via a lookup table. This was a suggestion by Kim and it worked out well.

Changes in detail:

• pre-NMT-init handling:

  • the new files nmtPreInit.hpp/cpp take case of NMT pre-init handling. They contain a small global lookup table managing C-heap blocks allocated in the pre-NMT-init phase.
  • os::malloc()/os::realloc()/os::free() defer to this code before doing anything else.
  • Please see the extensive comment block at the start of nmtPreinit.hpp explaining the details.

• Changes to NMT:

  • Before, NMT initialization was spread over two phases, initialize() and late_initialize(). Those were merged into one and simplified - there is only one initialization now which happens after argument parsing.
  • Minor changes were needed for the NMT_TrackingLevel enum - to simplify code, I changed NMT_unknown to be numerically 0. A new comment block in nmtCommon.hpp now clearly specifies what's what, including allowed level state transitions.
  • New utility functions to translate tracking level from/to strings added to NMTUtil
  • NMT has never been able to handle virtual memory allocations before initialization, which is fine since os::reserve_memory() is not called before VM parses arguments. We now assert that.
  • All code outside the VM handling NMT initialization (eg. libjli) has been removed, as has the code testing it.

• Gtests:

  • Some existing gtests had to be modified: before, they all changed global state (turning NMT on/off) before testing. This is not allowed anymore, to keep NMT simple. Also, this pattern disturbed other tests.
  • The new way to test is to passively check whether NMT has been switched on or off, and do tests accordingly: if on, full tests, if off, test just what makes sense in off-state. That does not disturb neighboring tests, gives us actually better coverage all around.
  • It is now possible to start the gtestlauncher with NMT on! Which additionally gives us good coverage.
  • To actually do gtests with NMT - since it's disabled by default - we now run NMT-enabled gtests as part of the hotspot jtreg NMT wrapper. This pattern we have done for a number of other facitilites, see all the tests in test/hotspot/jtreg/gtest.. . It works very well.
  • Finally, a new gtest has been written to test the NMT preinit lookup map in isolation, placed in gtest/nmt/test_nmtpreinitmap.cpp.

• jtreg:

  • A new test has been added, runtime/NMT/NMTInitializationTest.java, testing NMT initialization in the face of many many VM arguments.

---

Tests:

• ran manually all new tests on 64-bit and 32-bit Linux
• GHAs
• The patch has been active in SAPs test systems for a while now.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8256844: Make NMT late-initializable

Reviewers

• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to caf7652
• Zhengyu Gu (@zhengyu123 - Reviewer) ⚠️ Review applies to 42f2ca2

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4874/head:pull/4874
$ git checkout pull/4874

Update a local copy of the PR:
$ git checkout pull/4874
$ git pull https://git.openjdk.java.net/jdk pull/4874/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4874

View PR using the GUI difftool:
$ git pr show -t 4874

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4874.diff

[bridgekeeper]

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@tstuefe The following labels will be automatically applied to this pull request:

• core-libs
• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (ae56cb4d)
• 00: Full (42f2ca22)

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

Hi Thomas,

On 26/07/2021 10:15 pm, Thomas Stuefe wrote:

Short: this patch makes NMT available in custom-launcher scenarios and during gtests. It simplifies NMT initialization. It adds a lot of NMT-specific testing.

Before looking at this, have you checked the startup performance impact?

Thanks,
David
-----

[tstuefe]

Before looking at this, have you checked the startup performance impact?

Thanks,
David

Hi David,

performance should not be a problem. The potentially costly thing is the underlying hashmap. But we keep it operating with a very small load factor.

More details:

Adding entries is O(1). Since during pre-init phase almost only adds happen, startup time is not affected. Still, to make sure this is true, I did a bunch of tests:

• tested WCT of a helloworld, no differences with and without patch
• tested startup time in various of ways, no differences
• repeated those tests with 25000 (!) VM arguments, the only way to influence the number of pre-init allocations. No differences (VM gets slower with and without patch).

---

The expensive thing is lookup since we potentially need to walk a very full hashmap. Lookup affects post-init more than pre-init.

To get an idea of the cost of a too-full preinit lookup table, I modified the VM to do a configurable number of pre-init test-allocations, with the intent of artificially inflating the lookup table. Then, after NMT initialization, I measured the cost of lookup. The short story, I was not able to measure anything, even with a million pre-init allocations. Of course, with more allocations lookup table got fuller and the VM got slower, but the time increase was caused by the cost of the malloc calls themselves, not the table lookup.

Finally, I did an isolated test for the lookup table, testing pure adding and retrieval cost with artificial values. There, I could see costs for add were static (as expected), and lookup cost increased with table population. On my machine:

lu table entries	time per lookup
1000	3 ns
1 mio	240 ns

As you can see, if lookup table population goes beyond 1 mio entries, lookup time starts being noticeable over background noise. But with these numbers, I am not worried. Standard lookup population should be around 300-500, with very long command lines resulting in table populations of ~1000. We should never seen 10000 entries, let alone millions of them.

Still, I added a jtreg test to verify the expected hash table population. To catch errors like an unforeseen mass of pre-init allocations (lets say a leak or badly written code sneaked in), or if the hash algorithm suddenly is not good anymore.

Two more points

1. I kept this coding deliberately simple. If we are really worried about a degenerated lookup table, we can do things to fix that:

• we could automatically resize and rehash
• we could, if we sense something wrong, just stop filling it and disable NMT, stopping NMT init phase prematurely at the cost of not being able to use NMT.

The latter I had implemented already but removed it again to keep complexity down, and because I saw no need.

2. In our propietary production VM we have a system similar to NMT, but predating it. In that system we don't use malloc headers but store all (millions of) malloc'ed pointers in a big hash map. It performs excellent on all our libc variants. It is so fast that we just leave it always switched on. This solution has been productive since >10 years, and therefore I am confident that this is viable. This proposed hashmap with a planned population of 300-1000 is really not much :)

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

On 28/07/2021 12:17 am, Thomas Stuefe wrote:

On Mon, 26 Jul 2021 21:08:04 GMT, David Holmes <david.holmes at oracle.com> wrote:

Before looking at this, have you checked the startup performance impact?

Thanks,
David
-----

Hi David,

performance should not be a problem. The potentially costly thing is the underlying hashmap. But we keep it operating with a very small load factor.

More details:

Adding entries is O(1). Since during pre-init phase almost only adds happen, startup time is not affected. Still, to make sure this is true, I did a bunch of tests:

- tested WCT of a helloworld, no differences with and without patch
- tested startup time in various of ways, no differences
- repeated those tests with 25000 (!) VM arguments, the only way to influence the number of pre-init allocations. No differences (VM gets slower with and without patch).

----

The expensive thing is lookup since we potentially need to walk a very full hashmap. Lookup affects post-init more than pre-init.

To get an idea of the cost of a too-full preinit lookup table, I modified the VM to do a configurable number of pre-init test-allocations, with the intent of artificially inflating the lookup table. Then, after NMT initialization, I measured the cost of lookup. The short story, I was not able to measure anything, even with a million pre-init allocations. Of course, with more allocations lookup table got fuller and the VM got slower, but the time increase was caused by the cost of the malloc calls themselves, not the table lookup.

Finally, I did an isolated test for the lookup table, testing pure adding and retrieval cost with artificial values. There, I could see costs for add were static (as expected), and lookup cost increased with table population. On my machine:

| lu table entries | time per lookup |
| ------ |:-------------:|
| 1000 | 3 ns |
| 1 mio | 240 ns |

As you can see, if lookup table population goes beyond 1 mio entries, lookup time starts being noticeable over background noise. But with these numbers, I am not worried. Standard lookup population should be around *300-500*, with very long command lines resulting in table populations of *~1000*. We should never seen 10000 entries, let alone millions of them.

Still, I added a jtreg test to verify the expected hash table population. To catch errors like an unforeseen mass of pre-init allocations (lets say a leak or badly written code sneaked in), or if the hash algorithm suddenly is not good anymore.

Two more points

1) I kept this coding deliberately simple. If we are really worried about a degenerated lookup table, we can do things to fix that:
- we could automatically resize and rehash
- we could, if we sense something wrong, just stop filling it and disable NMT, stopping NMT init phase prematurely at the cost of not being able to use NMT.

The latter I had implemented already but removed it again to keep complexity down, and because I saw no need.

2) In our propietary production VM we have a system similar to NMT, but predating it. In that system we don't use malloc headers but store all (millions of) malloc'ed pointers in a big hash map. It performs excellent on *all our libc variants*. It is so fast that we just leave it always switched on. This solution has been productive since >10 years, and therefore I am confident that this is viable. This proposed hashmap with a planned population of 300-1000 is really not much :)

Thanks Thomas! I appreciate the detailed investigation.

Cheers,
David

[dholmes-ora]

Hi Thomas,

I had a look through this and it seems reasonable, but I'm not familiar enough with the details to approve at this stage.

A few nits below.

Thanks,
David

[coleenp]

This is an interesting and it seems a better way to solve this problem. Where were you all those years ago?? I hope @zhengyu123 has a chance to review it.
Also interesting is that we were wondering how we could return malloc'd memory on JVM creation failure, and this might partially help with that larger problem.

[openjdk]

@tstuefe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256844: Make NMT late-initializable

Reviewed-by: coleenp, zgu

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 78 new commits pushed to the master branch:

• 249d641: 8263561: Re-examine uses of LinkedList
• 6a3f834: 8268113: Re-use Long.hashCode() where possible
• 2536e43: 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt()
• 6c4c48f: 8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available
• 72145f3: 8269665: Clean-up toString() methods of some primitive wrappers
• 7cc1eb3: Merge
• e351de3: 8271272: C2: assert(!had_error) failed: bad dominance
• 6180cf1: 8271512: ProblemList serviceability/sa/sadebugd/DebugdConnectTest.java due to 8270326
• a1b5b81: 8271507: ProblemList SA tests that are failing with ZGC due to JDK-8248912
• 4bc9b04: 8263059: security/infra/java/security/cert/CertPathValidator/certification/ComodoCA.java fails due to revoked cert
• ... and 68 more: https://git.openjdk.java.net/jdk/compare/e4295ccfcdb16041d6f18fd64f7df3f740bf258f...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[tstuefe]

This is an interesting and it seems a better way to solve this problem. Where were you all those years ago?? I hope @zhengyu123 has a chance to review it.

Thank you! I was here, but we were not yet doing much upstream :) To be fair, this problem got quite involved and did cost me some cycles and false starts. I fully understand that the first solution uses the environment variable approach.

I spend some time investigating different ideas with this one; at first I did not use a hash-table but a static pre-allocated buffer from which I fed early allocations. But the code got too complex, and Kim's suggestion with the side table turned out just to be a lot simpler.

Also interesting is that we were wondering how we could return malloc'd memory on JVM creation failure, and this might partially help with that larger problem.

Yes, this would be trivial now, to return that memory. Though I am afraid it would be a small part only. But NMT may be instrumental in releasing all memory, since it knows everything. Only, its not always enabled.

Is that a real-life problem? Are there cases where the launcher would want to live on if the JVM failed to load?

[tstuefe]

Hi Thomas,

I had a look through this and it seems reasonable, but I'm not familiar enough with the details to approve at this stage.

A few nits below.

Thanks,
David

I did not expect a quick review for this one, so thanks for looking at this! All your suggestions make sense, I'll incorporate them.

..Thomas

[zhengyu123]

Sorry for late review.

Did a quick scan and have a few questions, will do more detail reading later.

[tstuefe]

Sorry for late review.

Did a quick scan and have a few questions, will do more detail reading later.

Thanks a lot, I appreciate your feedback!

[zhengyu123]

Looks good in general.

[tstuefe]

Hi Reviewers,

thanks for all the feedback!

New version:

• Found an embarrassing bug where I had forgotten to actually ::free preinit allocations. I went through the motions and all but forgot the final call. This would not have been a big problem, only a few scraps of memory lost, but still. Fixed with e2d40f2
• There is a piece of test code which I originally thought had to reside inside the hotspot but realized it did not. I removed it from the hotspot proper and moved it wholesale into a new gtest. caf7652
• I added another jtreg test to test that JDK tools can be called with -J-XX:NativeMemoryTracking and that NMT works dc34089
• Resolved @dholmes-ora feedback, including removing more obsolete code from libjli 48176dc
• Resolved feedback from @coleenp and @kimbarrett , fixing constness issue with NMTPreInitAllocationTable::find_entry(). I provided two versions based on const overloading. Changes trickle up the chain, but I think its better now (e.g. const find operation return const pointers) ef24874
• Resolved @zhengyu123 feedback and removed the post-init counters. ae56cb4

Thanks for your review work!

..Thomas

[coleenp]

Is that a real-life problem? Are there cases where the launcher would want to live on if the JVM failed to load?

There are a lot of other reasons why the launcher couldn't live on if the JVM fails to load. This was only one of them. We used to think about this problem once but don't really think it's solveable.

[coleenp]

This looks good. Thanks for fixing the mysterious (to me) cast.

[coleenp]

Oh, good. I was hoping this didn't need to be an in-jvm test.

[tstuefe]

This looks good. Thanks for fixing the mysterious (to me) cast.

Thank you, Coleen!

[tstuefe]

Nightlies at SAP showed no problems for several runs now. The failed GHA test (StringDeduplication) seems to have nothing to do with my patch.

@zhengyu123 are you fine with the latest version of this patch?

[zhengyu123]

Nightlies at SAP showed no problems for several runs now. The failed GHA test (StringDeduplication) seems to have nothing to do with my patch.

@zhengyu123 are you fine with the latest version of this patch?

Still good.

[tstuefe]

Thanks @coleenp and @zhengyu123 !

/integrate

[openjdk]

Going to push as commit eec64f5.
Since your change was applied there have been 113 commits pushed to the master branch:

• 4df1bc4: 6350025: API documentation for JOptionPane using deprecated methods.
• efcdcc7: 8270893: IndexOutOfBoundsException while reading large TIFF file
• 977b8c4: 8271836: runtime/ErrorHandling/ClassPathEnvVar.java fails with release VMs
• 04134fc: 8264543: Cross modify fence optimization for x86
• 9e76909: 8271824: mark hotspot runtime/CompressedOops tests which ignore external VM flags
• e49b7d9: 8271828: mark hotspot runtime/classFileParserBug tests which ignore external VM flags
• 68f7847: 8271825: mark hotspot runtime/LoadClass tests which ignore external VM flags
• 3d40cac: 8271821: mark hotspot runtime/MinimalVM tests which ignore external VM flags
• 33ec3a4: 8271744: mark hotspot runtime/getSysPackage tests which ignore external VM flags
• b48f31d: 8271743: mark hotspot runtime/jni tests which ignore external VM flags
• ... and 103 more: https://git.openjdk.java.net/jdk/compare/e4295ccfcdb16041d6f18fd64f7df3f740bf258f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tstuefe Pushed as commit eec64f5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.