RFR: 8305670: Performance regression in LockSupport.unpark with lots of idle threads [v4]

[Daniel D. Daugherty]

On Thu, 27 Apr 2023 02:25:19 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> src/hotspot/share/classfile/javaClasses.cpp line 1573:
>> 
>>> 1571: void java_lang_Thread::set_thread(oop java_thread, JavaThread* thread) {
>>> 1572:   java_thread->release_address_field_put(_eetop_offset, (address)thread);
>>> 1573: }
>> 
>> There is a convention that when we have acquire/release semantics, that we make that evident in the name of the method so that it is obvious at the call-site. So these would need renaming to `thread_acquire` and `release_set_thread`.
>
> Or, to avoid a lot of changes, add new methods for the acquire/release variants. Really only the setting to non-null requires release semantics. Which reads need acquire semantics is much harder to analyse - but most will not due to other inherent synchronization either via Threads_lock, or at the Java level.

I completely forgot about the `foo_acquire` and `release_set_foo` convention.
I think I'll go the route of adding `thread_acquire` and `release_set_thread` and
then will revisit each call-site to decide which to use.

I'm also going to look at splitting the `java_lang_Thread::thread_acquire` and
`java_lang_Thread::release_set_thread` changes into a sub-task. This will allow
for easier reviewing and backport decisions.

>> src/hotspot/share/prims/unsafe.cpp line 791:
>> 
>>> 789:     // early life stage of the JavaThread* is protected.
>>> 790:     FastThreadsListHandle ftlh(thread_oop, java_lang_Thread::thread(thread_oop));
>>> 791:     JavaThread* thr = ftlh.protected_java_thread();
>> 
>> I find this obscures things more than it aids understanding in this case. You don't need a special FTLH to get the desired semantics here.
>
> Note this is just a comment I'm not requesting you change it back.

Thanks for making it clear that the above is just a comment.

I rather like the fact that this is best encapsulation of this crazy performance
logic and it allows us to put (most of) the logic over in the threadSMR source
files where it belongs.

Of all the possible solutions we have tried out, I like this one the most and I
think that it's the least hacky. I wouldn't quite say that this solution "sings",
but it comes the closest (from my POV)...

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1179412244
PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1179417410