On Fri, 20 Dec 2024 15:33:29 GMT, Coleen Phillimore <cole...@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
