On Sat, 21 Dec 2024 07:16:22 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> move boostrapping_done flag into Mutex from MutexLocker
>
> src/hotspot/share/nmt/threadStackTracker.cpp line 55:
>
>> 53: align_thread_stack_boundaries_inward(base, size);
>> 54:
>> 55: MemTracker::MemTracker::NmtVirtualMemoryLocker nvml;
>
> Too many "MemTracker"s here?
thank you for spotting. I'll remove that
> src/hotspot/share/nmt/virtualMemoryTracker.cpp line 632:
>
>> 630: if (Mutex::is_bootstrapping_done()) {
>> 631: assert_lock_strong(NmtVirtualMemory_lock);
>> 632: }
>
> It seems like this can use `MemTracker::assert_locked()`, since that is
> conditioned on bootstrapping state.
`assert_lock_strong` skips checking the lock ownership if
`DebuggingContext::is_enabled() || VMError::is_error_reported()`.
`MemTracker::assert_locked()` does not do that. Is that ok?
> src/hotspot/share/runtime/mutex.hpp line 222:
>
>> 220: }
>> 221: private:
>> 222: static bool _bootstrapping_done;
>
> A description of what these mean would be nice. It seems to be mutex_init
> completed + threading
> initialization completed?
Yes, that's right. I'll add a comment describing this.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1895809641
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1895811421
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1895812737
