On Fri, 24 Mar 2023 10:47:11 GMT, Aleksey Shipilev <sh...@openjdk.org> 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