On Fri, 6 Sep 2024 20:18:36 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Comments. Hide assertion behind DEBUG. Replace MemoryFileTracker locker.
>> make reentrant.
>
> src/hotspot/share/nmt/nmtCommon.hpp line 168:
>
>> 166: intx const current = os::current_thread_id();
>> 167: intx const owner = Atomic::load(&_owner);
>> 168: assert(current == owner, "NMT lock should be acquired in this
>> section.");
>
> How about we try to re-use `assert_locked()` in constructor and destructor,
> something like:
>
>
> NmtGuard() {
> assert(!assert_locked(), "Lock is not reentrant");
> _nmt_semaphore.wait();
> Atomic::store(&_owner, current);
> }
>
> ~NmtGuard() {
> assert(assert_locked(), "NMT lock should be acquired in this section.");
> Atomic::store(&_owner, (intx) -1);
> _nmt_semaphore.signal();
> }
>
> static bool assert_locked(){
> intx const current = os::current_thread_id();
> intx const owner = Atomic::load(&_owner);
> return (current == owner);
> }
I think it's a good idea to use it in the destructor. But based on the
discussion above, it seems like we shouldn't assert the lock can't be reentered.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1748754688
