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.hol...@oracle.com>
Sent: Dienstag, 11. Februar 2020 08:44
To: Schmelter, Ralf <ralf.schmel...@sap.com>; Yasumasa Suenaga
<suen...@oss.nttdata.com>; OpenJDK Serviceability
<serviceability-dev@openjdk.java.net>
Cc: yasue...@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
