RFR 8240918: [REDO] Allow direct handshakes without VMThread intervention
Patricio Chilano
patricio.chilano.mateo at oracle.com
Thu Apr 2 19:59:03 UTC 2020
Hi Dan,
On 4/2/20 2:04 PM, Daniel D. Daugherty wrote:
>> 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()'.
Changed both.
> 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.
Changed both.
> L402: if ( (is_direct && op != _operation_direct)) {
> nit - please delete extra space after first '(' and the
> inner '(' and ')' are not needed.
Fixed.
> 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.
Done.
> 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/
Fixed.
> L4476: if (!ThreadsSMRSupport::is_a_protected_JavaThread((JavaThread
> *) thread)) {
> nit - The local 'thread' is a 'JavaThread*' so this cast
> should not be needed.
Fixed.
> 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.
Done.
> test/hotspot/jtreg/runtime/handshake/HandshakeDirectTest.java
> L26: * @test HandshakeDirectTest
> nit - there should there be an "@bug 8240918" after this line.
Done.
> L64: if (handshakee == me) {
> Please add a comment after this line:
> // Pick another thread instead of me.
Done.
> 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 '-'.
Done.
> 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.
It wasn't intentional actually but now that you mention it it's another
reason to not shut it down : )
> 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.
Great, thanks for reviewing this Dan!
Patricio
> 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