RFR: 8157023: Integrate NMT with JFR

[Stefan Johansson]

On Fri, 2 Dec 2022 09:03:50 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> src/hotspot/share/services/memReporter.cpp line 861:
>> 
>>> 859: 
>>> 860:   MemBaseline usage;
>>> 861:   usage.baseline(true);
>> 
>> Note that this is quite expensive. So it depends on how often we do this. How often are these samples taken?
>> 
>> Eg. Baseline.baseline does walk all thread stacks (so, probing all of them via mincore()). It also copies a lot of counters around. 
>> 
>> It is also not threadsafe. Are we at a safepoint here? Normally NMT reports are only done at safepoints.
>
> Haven't looked at the details of `baseline(bool summaryOnly)` that much, but since it `summaryOnly = true` I don't think it actually walk the thread stacks, right? 
> 
> We don't do this at a safepoint but looking at `MallocMemorySnapshot::copy_to(...)` it uses `ThreadCritical` to avoid things being cleaned out at the same time. Not sure if there are other thread safety problems though. I would expect this to have the same problems as a summary report triggered through `jcmd` because that isn't run at a safepoint either. But I do see that when used from `jcmd` we take a lock to serialize the NMT query, so we should probably do the same here.
> 
>   // Query lock is used to synchronize the access to tracking data.
>   // So far, it is only used by JCmd query, but it may be used by
>   // other tools.
>   static inline Mutex* query_lock() {
>     assert(NMTQuery_lock != NULL, "not initialized!");
>     return NMTQuery_lock;
>   }
> 
> So we certainly need to look closer at this. I would like to understand why the query lock is needed.

Looking a bit more at this. To me it seems like we are protecting the stored baseline with the lock is:

class MemTracker : AllStatic {
...
  // Stored baseline
  static MemBaseline      _baseline;
...
};

That the `NMTDCmd` code uses through `MemTracker::get_baseline()`. So if the JFR events keep its own baseline we should be fine without the lock. Or am I missing something here?

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

PR: https://git.openjdk.org/jdk/pull/11449