On Wed, 2 Oct 2024 13:28:13 GMT, Robert Toyonaga <d...@openjdk.org> wrote:
>> ### Summary >> This PR just replaces `ThreadCritical` with a lock specific to NMT. >> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT. >> I've implemented the new lock with a semaphore so that it can be used early >> before VM init. There is also the possibility of adding assertions in >> places we expect NMT to have synchronization. I haven't added assertions yet >> in many places because some code paths such as the (NMT tests) don't lock >> yet. I think it makes sense to close any gaps in locking in another PR in >> which I can also add more assertions. >> >> Testing: >> - hotspot_nmt >> - gtest:VirtualSpace >> - tier1 > > Robert Toyonaga has updated the pull request incrementally with one > additional commit since the last revision: > > Update src/hotspot/share/utilities/vmError.cpp > > Co-authored-by: David Holmes <62092539+dholmes-...@users.noreply.github.com> Hi Robert, sorry for the late answer, and thanks for your patience! src/hotspot/share/runtime/mutexLocker.cpp line 299: > 297: MUTEX_DEFN(ThreadsSMRDelete_lock , PaddedMonitor, > service-2); // Holds ConcurrentHashTableResize_lock > 298: MUTEX_DEFN(ThreadIdTableCreate_lock , PaddedMutex , safepoint); > 299: MUTEX_DEFN(SharedDecoder_lock , PaddedMutex , service-5); Why this? Do we print stacks under NMT lock protection? src/hotspot/share/utilities/vmError.cpp line 724: > 722: MemTracker::reduce_tracking_to_summary(); > 723: // Manually unlock if already holding lock upon entering error > reporting. > 724: NmtVirtualMemory_lock->unlock(); Thinking this through some more, I am now unsure about my old advice. I think if we force-unlock the mutex here, there should be no need for dropping the tracking mode to summary. Sorry if I gave conflicting advice before. So I think you could remove the reduce_tracking call (and its implementation). Dropping to summary has the disadvantage that it makes the NMT report in the hs-err file look like user ran with summary more active, which may confuse analysts. Force-unlocking is the way to go. ------------- PR Review: https://git.openjdk.org/jdk/pull/20852#pullrequestreview-2395460924 PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1816783901 PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1816797491
