RFR: 8273597: Rectify Thread::is_ConcurrentGC_thread()

[Coleen Phillimore]

On Fri, 10 Sep 2021 13:42:27 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> `Thread::is_ConcurrentGC_thread()` behaves differently to all other `Thread::is_xxx_thread()` functions, in the sense that it doesn't directly map to a distinct `Thread` sub-class. Instead, `is_ConcurrentGC_thread()` can today return true for both `ConcurrentGCThread` and `GangWorker`. These two classes have no super/sub-class relation. This is confusing and and potentially dangerous.
>> 
>> It would be reasonable to think that code like this would be correct:
>> 
>> 
>>   if (thread->is_ConcurrentGC_thread()) {
>>     conc_thread = static_cast<ConcurrentGCThread*>(thread);
>>     ...
>>   }
>> 
>> 
>> but it's not, since we might try to cast a `GangWorker` to a `ConcurrentGCThread`. And again, these two classes have no super/sub-class relation.
>> 
>> I propose that we clean this up, so that `is_ConcurrentGCThread()` only returns true for threads inheriting from `ConcurrentGCThread`. The main side-effect is that a handful of asserts need to be adjusted. In return, the code example above would become legal, and we can also remove some cruft from `WorkGang`/`GangWorker`.
>
> src/hotspot/share/code/nmethod.cpp line 1563:
> 
>> 1561:   DEBUG_ONLY(bool called_by_gc = Universe::heap()->is_gc_active() ||
>> 1562:                                  Thread::current()->is_ConcurrentGC_thread() ||
>> 1563:                                  Thread::current()->is_Worker_thread();)
> 
> Three places use the same condition. Did you consider creating a helper function?

I wonder if adding a helper function would encourage people to think they're the same again.

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

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