RFR: 8348347: Cleanup JavaThread subclass support in SA

[Chris Plummer]

On Wed, 5 Feb 2025 04:18:14 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

> Cleanup SA JavaThread support. Details in first comment:
> 
> Testing:
> - Tier1
> - Tier2 svc
> - Tier3
> - Tier5 svc
> - Local testing of debuggee using graal java compiler threads. Verified that the compiler threads shows up in jstack output if the property is set.

This PR addresses some errors and inconsistencies w.r.t. how SA handles the visibility of JavaThreads and their subclasses. There is some historic bit rot here that has resulted from old assumptions no longer being true.

In SA, Thread.isJavaThread() returned false for any JavaThread subclass. In hotspot it returns true for subclasses. The reason given in SA comments was essentially that JavaThread subclasses don't execute java code and will fail (with exceptions) if you try to get their java stack trace. Neither of those assumptions are true. There are JavaThread subclasses that execute java code (AttachListenerThread, NotificationThread, JvmtiAgentThread, and sometimes CompilerThread), and even those that don't won't cause SA any grief if you try to walk their stacks. You simply will not get any frames to walk. Thus in output like for jstack, you just see something like following, which is fine:


"Attach Listener" #19 daemon prio=5 tid=0x00007f0ab41f3d40 nid=18753 runnable [0x0000000000000000]
   java.lang.Thread.State: RUNNABLE
   JavaThread state: _thread_blocked


Hotspot has the concept of a JavaThread subclass being "hidden". Being hidden has many purposes in hostpot, most of which don't apply to SA. For example, JVMTI won't allow hidden threads to be suspended or resumed, or generate THREAD_START/END events for them. However, hotspot does omit hidden threads from stack dumps, so SA should do the same. All the thread types that normally never execute java code are considered hidden. This would include the following thread types: StringDedupThread, DeoptimizeObjectsALotThread, ServiceThread, MonitorDeflationThread and usually CompilerThread, unless it is executing a JVMCI compiler written in java.

I eventually realized that SA's isJavaThread() was not needed, and only isHiddenFromExternalView() is needed. So isJavaThread() is removed, which is good because of its confusing name. Also, some of the JavaThread subclasses needed to be fixed to return the correct value for isHiddenFromExternalView(), so that is a large part of what this PR does, but there is also a lot of other cleanup. Here's a breakdown of the changes:

- Remove isJavaThread(). Use isHiddenFromExternalView() when needed.
- Remove almost all JavaThread subclasses. They aren't needed because for the most part the only purpose of the subclass would be to override isHiddenFromExternalView() to return false. So instead of a bunch of JavaThread subclasses I added HiddenJavaThread to cover all the subclass types that are hidden. CompilerThread had to remain since it is the one subclass with some special SA functionality.
- Make needed adjustments to isHiddenFromExternalView()
- Get rid of a bunch of isXXXThread() apis. For the most part they were never called, and the one place where there was a valid call was actually suppose to be calling isHiddenFromExternalView().
- Cleanup comments regarding isJavaThread() and it being unsafe to try to access frames of threads that are not "java threads".
- In PointerLocation, which is used for findpc support, the logic for handling a pointer to a JNI handle was skipping providing thread details if the thread was a JavaThread subclass. There is no reason for this, so the isJavaThread() check is removed. Also removed the need for the JavaThread cast by changing handleThread from Thread to JavaThread.
- In StackTrace, which is used by jstack, isJavaThread() was used to filter out threads. This meant filtering out some java threads that were actually executing java. This check is replaced with !isHiddenFromExternalView().
- For SA heap dumping, it used to do two checks. The first was to exclude hidden threads. If not hidden then the 2nd was to exclude non-JavaThreads, which meant excluding all the JavaThread subclasses, even those that do execute java code. This was fixed by removing the isJavaThread() check. This check was being done in ThreadStackTrace.dumpStack(), which is only used when dumping the heap. I think it was a guard to protect against calling thread.getLastJavaVFrameDbg() on a native thread, which apparently in early SA days was not safe. Also in early SA days none of the JavaThread subclasses executed java, so skipping them was fine, but that is no longer the case.

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

PR Comment: https://git.openjdk.org/jdk/pull/23456#issuecomment-2635673154