On Wed, 25 Jan 2023 20:02:35 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> Roman Kennke has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert UseFastLocking to default to off
>
> src/hotspot/share/oops/markWord.hpp line 183:
>
>> 181: }
>> 182: markWord set_fast_locked() const {
>> 183: return markWord(value() & ~lock_mask_in_place);
>
> Perhaps add a comment above L183:
>
> // Clear the lock_mask_in_place bits to set locked_value:
Ok, I'm doing that.
> src/hotspot/share/runtime/lockStack.cpp line 34:
>
>> 32:
>> 33: LockStack::LockStack() :
>> 34: _base(UseFastLocking && !UseHeavyMonitors ? NEW_C_HEAP_ARRAY(oop,
>> INITIAL_CAPACITY, mtSynchronizer) : NULL),
>
> Okay so `UseFastLocking && UseHeavyMonitors`, then we don't need the lock
> stack.
Yeah if we do UseHeavyMonitors, we only ever lock using monitors. It probably
makes sense to reject the combination of flags? OTOH, -UseFastLocking
(currently implicit, using stack-locking) && +UseHeavyMonitors would not
complain? Seems odd, too.
> src/hotspot/share/runtime/lockStack.inline.hpp line 81:
>
>> 79: }
>> 80: }
>> 81: validate("post-contains");
>
> You only do the `validate("post-contains")` call when `false` is
> returned. Why not also validate for the `true` branch?
Good point. I am adding the validate call there, too.
> src/hotspot/share/runtime/synchronizer.cpp line 568:
>
>> 566: monitor->set_owner_from_anonymous(current);
>> 567: monitor->exit(current);
>> 568: }
>
> Hmmm... We're in `ObjectSynchronizer::exit()` so we should be the owner of
> the ObjectMonitor so I'm not yet sure what "Another thread beat us" means.
>
> XXX
See above. 'another thread beat us' refers to the preceding CAS, which can fail
if another thread inflated the monitor. I am rephrasing it to be more clear
(hopefully).
-------------
PR: https://git.openjdk.org/jdk/pull/10907
