RFR: 8286830: ~HandshakeState should not touch oops

[Patricio Chilano Mateo]

On Mon, 30 May 2022 21:51:37 GMT, David Holmes <dholmes at openjdk.org> wrote:

>>> @robehn Using on/off the ThreadsList does not seem right to me. Removal from the main thread-list happens much later than the point at which handshaking a terminating thread makes no sense; and removal from all threads-lists may not occur under we get into ThreadSMR::delete. To me it makes perfect sense for a thread to have a terminating state, like is_exiting, which effectively hides it from the rest of the VM in terms of interacting with it. To all and intents and purposes such a thread has already terminated as far as most of the outside world is concerned.
>> 
>> The state which hides it from the rest of the VM is the removal from the ThreadsList.
>> Since the JT after the state "is_exiting" still uses oops and interacts with Java, it's no where close to be hidden.
>> It participates in safepoint, still does JVM TI callbacks, it generates JFR events, etc.. (I guess posting contended monitor enter after posting thread end is a bug).
>> Basically the only subsystem that care about terminated flag is JVM TI and other 'serviceability'.
>> You can see the comment: "These things needs to be done while we are still a Java Thread"
>> And while we are a JavaThread we must participate in safepoints and handshakes.
>
>> Since the JT after the state "is_exiting" still uses oops and interacts with Java, it's no where close to be hidden.
> 
> Setting is_exiting is the start of hiding the thread from the rest of the system. The only interaction with "Java" after that is ensure_join to perform the ObjectMonitor notification and to mark the Thread's own Java state as "dead". This is the point at which this thread is dead to Java code and thus "hidden". The is_exiting state should be a gate for any JVMTI callbacks and JFR events as they don't apply to "terminated" threads - for JFR we call on_thread_exit shortly after setting the is_exiting state and just before that we could still post monitor exit events).
> 
> The thread must participate in safepoints up until its last oop-touch, which means after barrier_set->on_thread_detach() it does not need to participate in safepoints - and shouldn't hit any more polling points after that anyway.
> 
> A handshake with a thread is only needed to interact with that thread in some way and we don't interact with "terminated" threads - so no handshakes needed.
> 
> We have a two-stage termination sequence (is_exiting then terminated) precisely because the vast majority of interactions with a thread cease after is_exiting, and everything after "is terminated". So we're really only discussing the relatively small window between those two points. I'm arguing that there is no need to process handshakes from the start of that Window, rather than from the end. The basic sequence is:
> 
> 1. set is_exiting -> thread is no longer of interest to JVM TI or any handshakes related to JVM TI or Java code
> 2. set java.lang.Thread status to TERMINATED -> thread is now dead to Java
> 3. JFR::on_thread_exit() -> thread is now "dead" to JFR 
> 4. barrier_set()->on_thread_deatch() -> no more oop interaction so no need for further safepoints
> 5. ThreadSMRSupport::remove_thread -> thread no longer in main ThreadsList
> 6. set _thread_terminated 
> 
>> You can see the comment: "These things needs to be done while we are still a Java Thread"
> 
> That comment is referring to the thread destructor process where we must still be seen to be a JavaThread and not a basic Thread - ie those actions must happen before the end of ~JavaThread. (We had issues where we made calls to virtual functions from ~Thread which didn't dispatch correctly.)

> @dholmes-ora originally the idea was that if a JT is on a ThreadsList it will respond to handshakes. Note that while on the ThreadsList it may participate in safepoints. Since we also touch oops after "thread is exiting" we cannot stop responding to handshake after this stage. (ensure_join)
> 
> At one point I actually had a patch which removed the terminated state all together, and instead used on or off ThreadsList, which I think is the right approach, reducing complexity.
> 
> Anyhow a way longer discussion, this seems fine for now @pchilano , thanks!
>
Thanks for the review Robbin!

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

PR: https://git.openjdk.java.net/jdk/pull/8795