On Thu, 29 Jul 2021 22:53:42 GMT, Coleen Phillimore <cole...@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.
>
> 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?
[Not a review, just a drive-by comment.] It's casting away const. Better
would be a const_cast. And probably moved to the final result, with the body
keeping things const-qualified. And maybe const and non-const overloads of
this function. Or maybe this function shouldn't be const-qualified if a
non-const result is always needed, but that doesn't seem likely.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4874
