RFR: JDK-8281015: Further simplify NMT backend

[Zhengyu Gu]

On Mon, 31 Jan 2022 08:12:02 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> NMT backend can be further simplified and cleaned out.
> 
> - some entry points require NMT_TrackingLevel as arguments, some use the global tracking level. Ultimately, every part of NMT always uses the global tracking level, so in many cases the explicit parameter can be removed and the global tracking level can be used instead.
> - `MemTracker::malloc_header_size(level)` + `MemTracker::malloc_footer_size(level)` are fused into `MemTracker::overhead_per_malloc()`
> - when adding to `MallocSiteTable`, caller gets back a shortcut to the entry. That shortcut is stored verbatim in the malloc header. It consists of two 16-bit values (bucket index and chain position). That tupel finds its way into many argument lists. It can be simplified into single 32-bit opaque marker. Code outside the MallocSiteTable does not need to know what it is.
> - Currently, the `MallocHeader` class contains a lot of logic. It accounts (in constructor) and de-accounts (in `MallocHeader::release()`). It would simplify code if `MallocHeader` were just a dumb data carrier and the `MallocTracker` would do the actual work.
> - `MallocHeader` can be simplified, almost all members made constant and modifying accessors removed.
> - In some places we handle inputptr=NULL gracefully where we should assert instead
> - Expressions like `MemTracker::tracking_level() != NMT_off` can be simplified to `MemTracker::enabled()`.
> - MemTracker::malloc_base (all variants) can be removed. Note that we have MallocTracker::malloc_header, which achieves the same and does not require casting to the header.
> 
> Testing:
> 
> - GHAs
> - manually ran NMT gtests (all NMT modes) and NMT jtreg tests on Ubuntu x64
> - SAP nightlies ran through. Note that since 8275301 "Unify C-heap buffer overrun checks into NMT" NMT is enabled by default in debug builds, so it gets a lot more workout in tests now.
> 
> Note that I wanted to manually verify that the gdb "call pp" command still works in order to not break Zhengyu's recent addition, but found its already broken. I filed https://bugs.openjdk.java.net/browse/JDK-8281023 and am preparing a separate patch.

Overall is good, a few minor comments.

src/hotspot/share/services/mallocSiteTable.cpp line 161:

> 159: // Access malloc site
> 160: MallocSite* MallocSiteTable::malloc_site(uint32_t marker) {
> 161:   uint16_t bucket_idx = bucket_idx_from_marker(marker);

Please restore assert on bucket_idx.

src/hotspot/share/services/mallocTracker.hpp line 296:

> 294:   NOT_LP64(uint32_t _alt_canary);
> 295:   const size_t _size;
> 296:   const uint32_t _mst_marker;

make mst_marker a struct? instead of opaque type.

src/hotspot/share/services/memTracker.hpp line 115:

> 113:   static inline void* record_free(void* memblock, NMT_TrackingLevel level) {
> 114:     // Never turned on
> 115:     if (level == NMT_off || memblock == NULL) {

Wanna add assert `memblock != NULL`?

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

Marked as reviewed by zgu (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7283