RFR: 8346123: [REDO] NMT should not use ThreadCritical [v6]

[Robert Toyonaga]

On Fri, 20 Dec 2024 15:33:29 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Robert Toyonaga has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Revert to using NmtVirtualMemoryLocker. Use defaultStream. Add comment for SharedDecoder_lock
>
> src/hotspot/share/nmt/virtualMemoryTracker.cpp line 421:
> 
>> 419:   assert(_reserved_regions != nullptr, "Sanity check");
>> 420:   assert(!MemTracker::is_done_bootstrap() || NmtVirtualMemory_lock->owned_by_self() , "Should have acquired NmtVirtualMemory_lock");
>> 421: 
> 
> You could add this to MemTracker class like:
>  ```
>  inline static void assert_locked();
> 
> 
> Then put the body  
> 
> void MemTracker::assert_locked() {
>   assert(!is_bootstrapping_done() || NmtVirtualMemory_lock->owned_by_self(),
>             "should have acquired NmtVirtualMemory_lock");
> }
> 
> Then all these calls could be MemTracker::assert_locked().

Good idea. I've adopted your suggestion. Thank you!

> src/hotspot/share/nmt/virtualMemoryTracker.cpp line 631:
> 
>> 629: 
>> 630:   bool do_allocation_site(const ReservedMemoryRegion* rgn) {
>> 631:     assert_lock_strong(NmtVirtualMemory_lock);
> 
> So this is past bootstrapping?

I don't think this should ever get called during bootstrapping because thread stacks are only accounted lazily when a snapshot is created. I don't think an NMT snapshot would ever get created during bootstrapping, but just in case, I've added a check for `MemTracker::is_bootstrapping_done()`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1894218073
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1894217960