On Fri, 24 Mar 2023 10:47:11 GMT, Aleksey Shipilev <[email protected]> wrote:
>> Roman Kennke has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Merge remote-tracking branch 'origin/JDK-8291555-v2' into JDK-8291555-v2
>> - Set condition flags correctly after fast-lock call on aarch64
>
> src/hotspot/cpu/aarch64/aarch64.ad line 3954:
>
>> 3952: // Indicate success on completion.
>> 3953: __ cmp(oop, oop);
>> 3954: __ b(count);
>
> `aarch64_enc_fast_lock` explicitly sets NE on failure path. But this code
> just jumps to `no_count` without setting the flag. Does the code outside this
> encoding block rely on flags?
The code outside this encoding block relies on flags, yes. This is very ugly.
fast_unlock() jumps to no_count when the CAS fails, where the NE flag is set,
therefore we don't need to set it again.
> src/hotspot/share/runtime/synchronizer.cpp line 923:
>
>> 921: static bool is_lock_owned(Thread* thread, oop obj) {
>> 922: assert(UseFastLocking, "only call this with fast-locking enabled");
>> 923: return thread->is_Java_thread() ?
>> reinterpret_cast<JavaThread*>(thread)->lock_stack().contains(obj) : false;
>
> Here and later, should use `JavaThread::cast(thread)` instead of
> `reinterpret_cast<JavaThread*>`? It also sometimes subsumes the asserts, as
> `JT::cast` checks the type.
The problem is that the places where that helper function is used receive a
Thread* and not a JavaThread* (FastHashCode() and inflate()), and changing
those to accept JavaThread* percolates into various areas that I don't want to
touch right now (e.g. finalizerService.cpp). That is the reason why that
function exists to begin with. I'll do the changes that @shipilev suggested for
the time being. We may want to investigate restricting the incoming type in the
future.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1150580134
PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1150576745
