RFR: 8288497: add support for JavaThread::cannot_access_oops_safely() [v2]

[David Holmes]

On Thu, 16 Jun 2022 16:03:53 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> src/hotspot/share/runtime/thread.hpp line 1180:
>> 
>>> 1178:   bool is_exiting() const;
>>> 1179:   // thread's GC barrier is detached or thread is terminated
>>> 1180:   bool is_gc_barrier_detached() const;
>> 
>> Can we name this based on what it allows (access to oops) rather than what it relates to deep down? e.g. `can_access_oops_safely` or `is_oop_safe` ?
>
> We can definitely rename, but naming is hard... :-)
> 
> Those names you proposed are inversions of the current name and would
> require that the logic of the function be inverted.
> 
> The other names I came up before I settled on is_gc_barrier_detached():
> 
> 
>     can_no_longer_access_oops()
>     oops_are_no_longer_safe()
>     cannot_safely_access_oops()
> 
> 
> The closest match between your proposed names and mine are:
> 
> `can_access_oops_safely`  and `cannot_safely_access_oops`
> 
> Other opinions are welcome.

I think the style guide for naming is to use positive names to avoid double negatives when you have to negate the function - so `is_safe_for_x` rather than `is_not_safe_for_x`. I like `thread->is_oop_safe()` - short and clear.

>> src/hotspot/share/runtime/thread.hpp line 1186:
>> 
>>> 1184:   bool check_is_terminated(TerminatedTypes l_terminated) const {
>>> 1185:     return l_terminated != _not_terminated && l_terminated != _thread_exiting &&
>>> 1186:            l_terminated != _thread_gc_barrier_detached;
>> 
>> Existing: I don't think I buy this argument that we need to check all the states we don't want to see rather than the one we do. If the JavaThread has been freed we could see anything - and we should never be executing this code in the first place.
>
> The logic style of this function is a carry over from before we had Thread-SMR
> and we tried to detect a freed JavaThread. With Thread-SMR, we don't need to
> do the check this way at all.
> 
> Do we really want to change that as part of this fix for JDK19?

No it can be cleaned up later. This fix just highlighted the badness of that piece of code. :)

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

PR: https://git.openjdk.org/jdk19/pull/20