RFR: 8356173: Remove ThreadCritical [v2]

[Kim Barrett]

On Fri, 9 May 2025 22:09:09 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/memory/arena.cpp line 44:
>> 
>>> 42: // been created so we cannot use Mutex.
>>> 43: static PlatformMutex* _global_chunk_pool_mutex = nullptr;
>>> 44: static volatile int _recursion_count = 0;
>> 
>> Why is this volatile?  It's only accessed in the context of the lock.  Also, I think it probably doesn't
>> help for POSIX, since we're using `PTHREAD_MUTEX_NORMAL`, for which attempting recursion
>> "shall deadlock", with no error check.  (If you want error checking, you need to use
>> `PTHREAD_MUTEX_ERRORCHECK` and check the result of the lock attempt.)
>> 
>> I think the Windows critical sections that are used to implement PlatformMutex do allow recursion,
>> and maybe we should have this kind of guard to prevent recursive use there, to be consistent with
>> the restriction imposed by the POSIX implementation.  (Or change the POSIX implementation to
>> use `PTHREAD_MUTEX_RECURSIVE`, but I don't think we should do that.)
>> 
>> Maybe the POSIX PlatformMutex should use `PTHREAD_MUTEX_ERRORCHECK` in debug
>> builds.  But that's a separate issue.
>
> You're right, it doesn't need to be volatile. I want the recursion checking to be platform independent for this lock, and not affect the other PlatformMutex/Monitors which are outside the scope of this change and hopefully are what we want already.

But the recursion checking isn't platform independent.  For POSIX it does nothing useful and is just useless
clutter providing a false sense of security.  I think it should be removed, and consider (as a separate thing)
adding a recursion assert to the Windows implementation.

And it should be debug-only there; there's no need to change the counter in a release build, since the
check is an assert.  It also doesn't need to be a counter; it could be a bool flag.  But that's really about a
future Windows-check.  It just shouldn't be here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25072#discussion_r2082793891