RFR: JDK-8261644: NMT: Simplifications and cleanups

[Thomas Stuefe]

On Fri, 12 Feb 2021 22:17:43 GMT, Coleen Phillimore <coleenp 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
>
> src/hotspot/share/services/mallocSiteTable.cpp line 141:
> 
>> 139:   while (head != NULL && (*pos_idx) <= MAX_BUCKET_LENGTH) {
>> 140:     MallocSite* site = head->data();
>> 141:     if (site->equals(key, flags)) {
> 
> Does this now assert when it used to just return false if memflags didn't match?

Yes, and it seems this actually fired tonight on s390x. Which indicates that the call stack frame skipping is off and we do skip too much. I opened https://bugs.openjdk.java.net/browse/JDK-8261556 to track this, and here I will restore the old behavior for now. That is also cleaner since this RFE should not have brought behavioral changes.

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

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