RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump

[David Holmes]

Hi Ralf,

On 12/02/2020 6:41 pm, Schmelter, Ralf wrote:
> Hi David,
> 
>> I see little point in subclassing NonJavaThread (via NamedThread) but
>> then overriding pre_run() and post_run() so that you don't do anything
>> that NonJavaThread is supposed to do regarding the NJT iterator
>> capabilities.
> 
> The problem is  the post_run() method of NamedThread calls Thread::clear_thread_current(), which then makes it impossible to delete the thread at least in a debug build, since the code in ~Thread calls os::free_thread() which calls Thread::current()->->osthread() in an assert, which obviously will crash.

If the lifecycle of your new NonJavaThread does not fit the existing 
NonJavaThreads then yes you will need to override pre_run() and 
post_run(), but you shouldn't just delete all the NJT iteration support 
- at least it isn't obvious to me that it is valid to do so.

> Originally I tried not use my own threads at all and instead use the WorkGang from CollectedHeap:: get_safepoint_workers(). But this ultimately failed because I'm not allowed to iterate the heap in a worker thread on Shenandoah. Additionally ParallelGC did not implement get_safepoint_workers(), but that should have not been a problem.

That begs the question for me exactly what it is that your new NJT 
worker thread will touch in the VM because that will determine where it 
needs to fit in the Thread hierarchy and what actions it needs to 
perform in pre_run() and post_run(). I'm unclear what state the VM will 
be in when this heap dump is performed and these worker threads are 
doing the compression.

Thanks,
David

> Maybe it is better to try to get this to work (e.g. if I could specify a foreground task when calling run_task(), the problem could be avoid by doing the iteration in the foreground task). But I'm not sure how changes in this area are seen.
> 
>> For your monitor operations, you should use a MonitorLocker and then
>> call ml->wait() which will do the right thing with respect to "no
>> safepoint checks" without you needing to specify it directly.
> 
> Thanks, will do.
> 
> Best regards,
> Ralf
> 
> -----Original Message-----
> From: David Holmes <david.holmes at oracle.com>
> Sent: Dienstag, 11. Februar 2020 08:44
> To: Schmelter, Ralf <ralf.schmelter at sap.com>; Yasumasa Suenaga <suenaga at oss.nttdata.com>; OpenJDK Serviceability <serviceability-dev at openjdk.java.net>
> Cc: yasuenag at gmail.com
> Subject: Re: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
> 
> Hi again Ralf, :)
> 
> A few more comments after taking a closer look at the thread code.
> 
> On the surface it seems to me this is a case where it would be okay to
> introduce a subclass of Thread that is not JavaThread nor NonJavaThread.
> I see little point in subclassing NonJavaThread (via NamedThread) but
> then overriding pre_run() and post_run() so that you don't do anything
> that NonJavaThread is supposed to do regarding the NJT iterator
> capabilities. But we currently expect all threads to fit into one
> category or another, so this is problematic. :( I thinking disabling the
> NJT functionality is also problematic. So not sure what to suggest yet.
> 
> BTW you extended NamedThread but you never actually set a name AFAICS. ??
> 
> For your monitor operations, you should use a MonitorLocker and then
> call ml->wait() which will do the right thing with respect to "no
> safepoint checks" without you needing to specify it directly.
> 
> Cheers,
> David
>