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

[Schmelter, Ralf]

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.

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.

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