On Wed, 13 Mar 2024 20:01:22 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>> We've received feedback from users of cloud APM platform wanting the new
>> version of the JDK to allow the HotSpotDiagnosticMXBean.dumpHeap
>> underpinnings to reduce STW time using sgemented heapdump. Supporting
>> segmented heapdump for mxbean is a reasonable requirement and adds little
>> additional complexity, both in terms of maintainability and understanding.
>
> Hi!
>
> Segmented is an awkward term as we use that word inside hprof too. For the
> previous change we used the phrase "two phase" as well as calling it
> segmented, which I think helps.
>
> Also this change means jmm_DumpHeap will always use
> HeapDumper::default_num_of_dump_threads().
>
> That's not a bad thing, but could the bug title be something like "Enable
> two-phase heap dump for HotSpotDiagnosticMXBean (JMX)" ? I think then it's
> clearer what's happening just from the title.
>
>
>
> + if (current_thread->is_AttachListener_thread() || !_oome) {
>
> I think "!_oome" here means "we are called from JMX" ? We should clarify.
> 8-) Trying to suggest a way of describing this.
>
> // Operating in attach listener or called by management library:
> // perform heapdump file merge operation in the current thread,
> preventing us
> // from occupying the VM Thread, which in turn affects the occurrence of
> // GC and other VM operations.
>
> So the other case is only HeapDumpOnOutOfMemoryError processing, where we do
> the merge in a VM operation? Just checking if that's all agreed. 8-)
Hi @kevinjwalls Thanks for your review! I was reminded by Alex that there is
already a [similar issue](https://bugs.openjdk.org/browse/JDK-8322043) here
ready to make two phase heap dump the default choice, so all we need is to
simply change the default parallel parameter (and potential bug fix)
https://github.com/openjdk/jdk/blob/21279b1f652c54693e317d55e7da6478b959fdfb/src/hotspot/share/services/heapDumper.hpp#L64
I will first synergize with @alexmenkov to see if we can improve based on the
current patch, or close the current PR and wait for his new one.
P.S. the phrase "two phase" makes sense to me:)
-------------
PR Comment: https://git.openjdk.org/jdk/pull/18221#issuecomment-1996279522
