On Thu, 22 Jul 2021 14:58:47 GMT, Thomas Stuefe <stu...@openjdk.org> 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, 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.
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.
src/hotspot/share/services/nmtPreInit.hpp line 128:
> 126: // Returns start of the user data area
> 127: void* payload() { return this + 1; }
> 128: const void* payload() const { return this + 1; }
This is an odd looking overload (that I wouldn't have thought possible). Maybe
one of these payloads can be renamed to why it's const.
src/hotspot/share/services/nmtPreInit.hpp line 166:
> 164: NMTPreInitAllocation** find_entry(const void* p) const {
> 165: const unsigned index = index_for_key(p);
> 166: NMTPreInitAllocation** aa = (NMTPreInitAllocation**)
> (&(_entries[index]));
Why is this cast needed?
-------------
Marked as reviewed by coleenp (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4874
