RFR 8240918: [REDO] Allow direct handshakes without VMThread intervention
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Apr 2 17:04:22 UTC 2020
> Full: http://cr.openjdk.java.net/~pchilanomate/8240918/v2/webrev/
Thanks for redoing to this patch!
src/hotspot/share/runtime/handshake.hpp
L85: void set_thread(JavaThread* thread) { _handshakee = thread; }
nit - usually the 'setter' name matches the field name. So this
could/should
be called 'set_handshakee()'.
L103: Thread* get_active_handshaker() const { return
_active_handshaker; }
nit - usually 'get_' is omitted from the 'getter'. So this
could/should
be called 'active_handshaker()'.
src/hotspot/share/runtime/handshake.cpp
L56: assert(val >= 0, "_pending_threads cannot be negative");
Perhaps:
assert(val >= 0, "_pending_threads=%d: cannot be
negative", val);
Gives a little better debug info.
L67: assert(_pending_threads == 0, "Must be zero");
Perhaps:
assert(_pending_threads == 0, "Must be zero: %d",
_pending_threads);
Gives a little better debug info.
L402: if ( (is_direct && op != _operation_direct)) {
nit - please delete extra space after first '(' and the
inner '(' and ')' are not needed.
src/hotspot/share/runtime/threadSMR.hpp
No comments.
src/hotspot/share/runtime/thread.hpp
L1359: Thread* get_active_handshaker() const {
nit - If you rename 'get_active_handshaker()' to
'active_handshaker()' as
mentioned above, you should also rename this one.
src/hotspot/share/runtime/thread.cpp
L4472: // We must check there are no active references to this thread
grammar - s/check there/check that there/
L4476: if
(!ThreadsSMRSupport::is_a_protected_JavaThread((JavaThread *) thread)) {
nit - The local 'thread' is a 'JavaThread*' so this cast should
not be needed.
L4479-82 - Thanks for the good comment about _thread_key in TLS.
src/hotspot/share/runtime/safepointMechanism.inline.hp
No comments.
src/hotspot/share/runtime/safepoint.cppp
No comments.
src/hotspot/share/runtime/biasedLocking.cpp
No comments.
src/hotspot/share/runtime/mutexLocker.cpp
L191: if (Thread::current() == thread->get_active_handshaker())
return;
nit - If you rename 'get_active_handshaker()' to
'active_handshaker()' as
mentioned above, you should also rename this one.
test/hotspot/jtreg/runtime/handshake/HandshakeDirectTest.java
L26: * @test HandshakeDirectTest
nit - there should there be an "@bug 8240918" after this line.
L64: if (handshakee == me) {
Please add a comment after this line:
// Pick another thread instead of me.
L63: int handshakee =
ThreadLocalRandom.current().nextInt(0, WORKING_THREADS-1);
L111: int i =
ThreadLocalRandom.current().nextInt(0, WORKING_THREADS-1);
nit - please add spaces around '-'.
L107: Thread suspendResumeThread = new Thread() {
So you don't do anything to shutdown the suspend-resume thread
which will stress the use of suspend/resume at workingThreads
exit time. I like it and I hope that's intentional.
Thumbs up! My comments above are nits so it is your call whether to address
them or not. I do not need to see a new webrev.
Dan
On 4/2/20 11:11 AM, Patricio Chilano wrote:
> Hi David,
>
> On 4/2/20 4:45 AM, David Holmes wrote:
>> Hi Patricio,
>>
>> On 2/04/2020 3:45 am, Patricio Chilano wrote:
>>> Hi all,
>>>
>>> Please review this redo for 8230594. Since the reason behind the
>>> Windows timeouts turned out to be 8240902, this patch is almost
>>> identical to the reverse of the backout modulo the rebase.
>>
>> Glad to see this come back! Overall changes still look good - naturally.
>>
>>> There is only one small fix for an issue not identified in the
>>> original patch which I explained in the comments. To avoid this
>>> possible issue of deleting a JavaThread that called
>>> Thread::destroy_vm() while there are handshakers blocked in the
>>> handshake_turn_sem semaphore, I added a check to verify the
>>> JavaThread in question is not protected by some ThreadsList
>>> reference before attempting the delete it. In case it is protected,
>>> we need to at least clear the value for _thread_key to avoid
>>> possible infinite loops at thread exit (this can happen in Solaris).
>>
>> Interesting problem. At some point during the shutdown sequence the
>> thread needs to be able to execute "process any pending handshake and
>> mark this thread as no longer handshake-capable". But it is unclear
>> how to achieve that at this time so I'm okay with deferring this to a
>> future RFE. Even though the VM has "terminated" this may be a custom
>> launcher, or a hosted VM, and the main process is not necessarily
>> going away, so it would be preferable to be able to delete the thread
>> and free all its resources.
> Ok, deferring this to a future RFE sounds good. I actually tried to
> think how to safely delete the thread even in these cases but I can't
> find a clean way without complicating too much the code. Since this is
> a special case and only when the VM has already terminated I thought
> this was the best approach. I see your point about the hosted VM case
> though. But also as Coleen pointed out we are not freeing a lot of
> other threads too so I don't think not freeing one extra thread will
> hurt. Just to highlight that, before the main thread exits I see still
> alive, not counting Compiler and GC threads, Finalizer, Reference
> Handler, Signal Dispatch, Service Thread, Sweeper thread,
> Common-Cleaner and Notification Thread. And although I don't see the
> VMThread we are not actually deleting it. This is the comment at the
> end of VMThread::run():
>
> // We are now racing with the VM termination being carried out in
> // another thread, so we don't "delete this". Numerous threads don't
> // get deleted when the VM terminates
>
> I'm not saying we shouldn't try to delete this thread because we are
> not doing it with other threads though. But I think in this special
> case keeping the code simpler is probably worth more than complicating
> the code to free one extra thread.
>> One nit: in the ThreadSMR code this looks really odd:
>>
>> static bool is_a_protected_JavaThread_with_lock(JavaThread *thread,
>> bool skiplock = false);
>>
>> A "with lock" function that might not lock? I suggest just making
>> is_a_protected_JavaThread public and calling it directly.
> Changed. Originally I didn't want to declare
> is_a_protected_JavaThread() public to make it clear that
> is_a_protected_JavaThread_with_lock() is the one that should be always
> called. But there is an assertion that we either own the Threads_lock
> or we are at a safepoint, so if somebody tries to call this in an
> illegal scenario it should assert.
>
> Here is v2:
>
> Full: http://cr.openjdk.java.net/~pchilanomate/8240918/v2/webrev/
> Inc: http://cr.openjdk.java.net/~pchilanomate/8240918/v2/inc/webrev/
>
> Thanks for looking at this David!
>
> Patricio
>> Thanks,
>> David
>> -----
>>
>>> The only other change is that I removed the check for local polls in
>>> Handshake::execute_direct() since after the backout, thread-local
>>> handshakes was implemented for arm32 which was the only remaining
>>> platform.
>>>
>>> I tested it several times in mach5 tiers 1-6 and once in t7 and saw
>>> no failures.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8240918
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~pchilanomate/8240918/v1/webrev/
>>>
>>>
>>> Thanks,
>>> Patricio
>
More information about the hotspot-runtime-dev
mailing list