RFR: 8277486: NMT: Cleanup ThreadStackTracker

[Zhengyu Gu]

On Wed, 24 Nov 2021 08:51:26 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> src/hotspot/share/services/threadStackTracker.cpp line 103:
>> 
>>> 101:         assert(MemTracker::tracking_level() < NMT_summary, "Must be");
>>> 102:         return false;
>>> 103:       }
>> 
>> I looked at `MemTracker::transition_to`, the downgrade path:
>> 
>> 
>>   } else if (current_level > level) {
>>     // Downgrade tracking level, we want to lower the tracking level first
>>     _tracking_level = level;
>>     // Make _tracking_level visible immediately.
>>     OrderAccess::fence();
>>     VirtualMemoryTracker::transition(current_level, level);
>>     MallocTracker::transition(current_level, level);
>>     ThreadStackTracker::transition(current_level, level);
>> 
>> Is the comment about "immediately" correct? I would have assumed OrderAccess::fence() has not have anything to do with timing, just with the order the writes are perceptible from outside?
>> 
>> So, concurrent threads may observe tracking level == minimal but the pointers to the infrastructure are still there?
>> 
>> I see that you do not guard `MemTracker::transition_to` with ThreadCritical. You do guard teardown inside `ThreadStackTracker::transition` and `VirtualMemoryTracker::transition` but it looks like teardown in `MallocTracker::transition` is unguarded by ThreadCritical at least.
>> 
>> Just a proposal, would guarding upstairs in `MemTracker::transition_to` not be clearer and safer, especially if it includes changing the tracking level?
>> 
>> Alternatively, do we even have to delete all those structures on shutdown? They are not that costly. We shut down either manually - nobody cares then - or if we encounter native OOM. In the latter case, those detail structures are arguably important since they contain data that may help analyze the OOM. The malloc site table, for instance.
>
> To add to that, I always wondered why we even expose `shutdown` via jcmd... especially since this is a one-way operation, so its use is limited.

`ThreadStackTracker` could be tracked as `malloc` memory, and yes, `MallocTracker::transition` is not guarded by `ThreadCritical`, because it is too expensive on other operations.

Yes, I believe we have yet seen any bug reported, solely due to limited use of shutdown function :-)

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

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