RFR: JDK-8261644: NMT: Simplifications and cleanups

[Thomas Stuefe]

On Fri, 12 Feb 2021 07:16:45 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> Hi,
> 
> may I please have reviews for this RFE?
> 
> While working on NMT I found a number of possible cleanups and simplifications. I avoided mixing these cleanups with fixed and instead put them into this cleanup RFE.
> 
> - de-templatize `AllocationSite<E>` since E was used as simple data holder for child classes; the same effect can be had with traditional inheritance with less and clearer code (also IDEs get less confused)
> 
> - `AllocationSite` child classes `SimpleThreadStackSite`, `VirtualMemoryAllocationSite`, `MallocSite` were simplified.
> 
> - As for `SimpleThreadStackSite`, we can get rid of the separate data holder class `SimpleThreadStack` entirely by merging its members directly into `SimpleThreadStackSite`. In theory we could do the same for the data holder classes `MemoryCounter` and `VirtualMemory` for `MallocSite` and `VirtualMemoryAllocationSite` too but this would cause larger ripples so I stopped there.
> 
> - removed the SimpleThreadStackSite(address base, size_t size) constructor (the one not taking a call stack) by slightly rewriting its sole user
> 
> - made `AllocationSite` immutable
> 
> - removed unused default constructors from `MallocSite` and `MallocSiteHashTableEntry` since they were not needed
> 
> - removed unused methods `set_callsite()`, `hash()`, `equals()` from `MallocSiteHashTableEntry`
> 
> - There was a subtle incorrectness where `AllocationSite::equals()` would only compare callstack and disregard the MEMFLAGS member. Theoretically, if two callstacks end with the same lowest frame, they should always reference the same single allocation, so that's okay. But if the call stack capturing was not precise enough (eg skipping too many low frames) we may accidentally lump several allocation sites together which could have different MEMFLAGS. I added an assert to check that.
> 
> - `NativeCallStack`: Removed the `fillStack` argument from the first constructor to avoid having to evaluate it in this hot constructor. Its true in almost all cases.
> 
> - Also removed the `toSkip` default value. Instead, I added an explicit default constructor.
> 
> - Moved the malloc site table tuning statistics printing from memtracker.cpp down into a new function `MallocSiteTable::print_tuning_statistics()`. When implemented inside `MallocSiteTable`, that coding does not need a walker object anymore and becomes a lot simpler. In particular, we don't have to rely on implicit knowledge about walking order, which made the code complex and was vulnerable against subtle errors. New code is more compact and simpler.
> ----
> Tests: 
> - github GA
> - manual NMT jtreg tests (including the currently disabled runtime/NMT/CheckForProperDetailStackTrace.java)
> - Full nightlies at SAP are scheduled

> @tstuefe - Thanks for keeping cleanups separated from bug fixes. That always
> makes the PR reviews easier. I don't see any information about what testing was
> done on this PR.

Thanks Dan. I updated the description. I'm very careful after JDK-8261520. Manual tests went through, nightlies at SAP are scheduled. BTW this patch is a preparation for the fix for JDK-8261520.

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

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