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.
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
src/hotspot/share/services/nmtCommon.hpp line 127:
> 125: }
> 126:
> 127: // parses the tracking level from a string. Returns NMT_unknown if
Nit: start sentence with capital P
src/hotspot/share/services/nmtCommon.hpp line 131:
> 129: static NMT_TrackingLevel parse_tracking_level(const char* s);
> 130:
> 131: // returns textual representation of a tracking level.
Nit: start sentence with capital R
src/hotspot/share/services/nmtPreInit.hpp line 114:
> 112: //
> 113: // We use a basic open hashmap, dimensioned generously - hash collisions
> should be very rare.
> 114: // The table is customized for holding malloced pointers. One main
> point of this map is that we do
Nit: double-space at line start (and for rest of comment block)
src/hotspot/share/services/nmtPreInit.hpp line 140:
> 138:
> 139: // table_size: keep table size a prime and the hash function simple;
> this
> 140: // seems to give a good distribution for malloce'd pointers on all
> our libc variants.
typo: malloce'd
src/hotspot/share/services/nmtPreInit.hpp line 268:
> 266: add_to_map(a);
> 267: (*rc) = a->payload();
> 268: _num_mallocs_pre ++;
style nit: no space with unary operator
src/java.base/share/native/libjli/java.c line 807:
> 805: */
> 806: static void
> 807: SetJvmEnvironment(int argc, char **argv) {
This doesn't seem to do anything any more.
test/hotspot/gtest/nmt/test_nmtpreinitmap.cpp line 2:
> 1: /*
> 2: * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights
> reserved.
New file should only have single, current, copyright year
-------------
PR: https://git.openjdk.java.net/jdk/pull/4874
