RFR: JDK-8261644: NMT: Simplifications and cleanups [v4]

[Coleen Phillimore]

On Sat, 13 Feb 2021 05:43:56 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.
>> 
>> There should be no behavioral changes in this patch.
>> 
>> - 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. (_Update: seems this assert really fires on s390x, so this is a real problem. I opened [1] to track this and restored the old behavior._).
>> 
>> - `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. Before removing the old implementation, I ran both statistics side by side for a couple of scenarios (eg really bad hash code implementations) and the output was identical.
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8261556
>> 
>> ----
>> Tests: 
>> - github GA
>> - manual NMT jtreg tests (including the currently disabled runtime/NMT/CheckForProperDetailStackTrace.java)
>> - Full nightlies at SAP are scheduled
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   reduce diff

Looks good!

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

Marked as reviewed by coleenp (Reviewer).

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