RFR: 8356173: Remove ThreadCritical [v2]

[Kim Barrett]

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

>> Updated the description in the bug.  This removes the last use of ThreadCritical and replaces it with a global PlatformMutex lock.
>> Tested with tier1-4, and tier1 on all Oracle-supported OSs.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add recursion detection.

Changes requested by kbarrett (Reviewer).

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.

src/hotspot/share/nmt/nmtUsage.cpp line 61:

> 59:     ChunkPoolLocker lock;
> 60:     ms = MallocMemorySummary::as_snapshot();
> 61:   }

Shortening the scope of the lock seems contrary to the comment explaining its presence.

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

PR Review: https://git.openjdk.org/jdk/pull/25072#pullrequestreview-2829587653
PR Review Comment: https://git.openjdk.org/jdk/pull/25072#discussion_r2082462737
PR Review Comment: https://git.openjdk.org/jdk/pull/25072#discussion_r2082453048