RFR 8230594: Allow direct handshakes without VMThread intervention
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jan 16 22:54:37 UTC 2020
On 1/16/20 5:40 PM, David Holmes wrote:
> Hi Dan,
>
> Can I pick up on one suggestion you made:
>
> L341: assert(Thread::current() == _thread, "should call from
> thread");
> This should probably be JavaThread::current().
>
> If we get the wrong thread such that the assert fails, and that thread
> is not even a JavaThread, then we will get an assertion failure inside
> JavaThread::current() instead of failing the L341 assertion.
No problem with correcting my comment. You are right about where we'll
fail when JavaThread::current() is called by a non-JavaThread. I don't
see that as much of a problem since it will make it clear that
JavaThread::current() was called unexpectedly by a non-JavaThread and
the call trace will lead to the original assertion. YMMV...
> So I suggest that Thread::current() is always the best method to use
> in an assertion involving thread identity.
I might be a bit tired/fried at this point, but what do you
think is the use case for JavaThread::current()?
Dan
>
> Cheers,
> David
>
> On 17/01/2020 8:23 am, Daniel D. Daugherty wrote:
>>> Here is v3:
>>> Full: http://cr.openjdk.java.net/~pchilanomate/8230594/v03/webrev/
>>
>> src/hotspot/share/runtime/handshake.hpp
>> L34: class HandshakeOperation;
>> nit: please move before L33 for alpha sort order.
>>
>> L38: // either by the thread itself or by the VM thread while
>> keeping the thread
>> nit: s/VM thread/VMThread/
>>
>> Not your typo, but for consistency with your comment addition.
>> (Also, I prefer "VMThread" to "VM thread".)
>>
>> L41: // thread itself or, depending on whether the operation is
>> a direct handshake
>> Perhaps:
>> s/thread/target thread/
>> or:
>> s/thread/target JavaThread/
>>
>> since only JavaThreads can be the target of a handshake.
>>
>> L42: // or not, by the JavaThread that requested the handshake
>> or the VMThread
>> Can handshakes be requested by non-JavaThreads?
>>
>> Perhaps this for the comment block:
>>
>> L36: // A handshake closure is a callback that is executed for
>> each JavaThread
>> L37: // while that thread is in a safepoint safe state. The
>> callback is executed
>> L38: // either by the target JavaThread itself or by the
>> VMThread while keeping
>> L39: // the target thread in a blocked state. A handshake can be
>> performed with a
>> L40: // single JavaThread as well. In that case, the callback is
>> executed either
>> L41: // by the target JavaThread itself or, depending on whether
>> the operation is
>> L42: // a direct handshake or not, by the thread that requested
>> the handshake or
>> L43: // the VMThread respectively.
>>
>>
>> L65: // the JavaThread or the JavaThread itself.
>> nit: s/or the/or by the target/
>>
>> src/hotspot/share/runtime/handshake.cpp
>> L174: _op->add_target_count(number_of_threads_issued - 1);
>> Perhaps add a comment above the line:
>> // _op was created with a count == 1 so don't double count:
>>
>> L285: JavaThread *self = (JavaThread*)Thread::current();
>> L325:
>> _handshake_turn_sem.wait_with_safepoint_check((JavaThread*)Thread::current());
>>
>> If the caller is supposed to be a JavaThread, then use
>> JavaThread::current() instead of casting.
>>
>> L286: HandshakeOperation op(thread_cl, true);
>> Please prefix literal booleans with /*is_direct*/ so we know
>> what
>> the parameter is.
>>
>> L341: assert(Thread::current() == _thread, "should call from
>> thread");
>> This should probably be JavaThread::current().
>>
>> L356: // Disarm before execute the operation
>> L362: // Disarm before execute the operation
>> nit: s/execute/executing/
>>
>> L357: clear_handshake(false);
>> L363: clear_handshake(true);
>> Please prefix literal booleans with /*is_direct*/ so we know
>> what
>> the parameter is.
>>
>> L430: if ( (is_direct && op != _operation_direct)) {
>> L431: _processing_sem.signal();
>> Why is the signal() call needed here (and not in the other
>> 'return false' cases above)?
>>
>> src/hotspot/share/runtime/biasedLocking.cpp
>> L671: assert(Thread::current() == biased_locker ...
>> Can you add:
>>
>> Thread* cur = Thread::current();
>>
>> above this line? Then switch all the uses of
>> "Thread::current()"
>> in this function to "cur".
>>
>> src/hotspot/share/runtime/mutexLocker.cpp
>> L191 void assert_locked_or_safepoint_or_handshake(const
>> Mutex* lock, const JavaThread* thread) {
>> old L192: if (Thread::current()->is_VM_thread() &&
>> thread->is_vmthread_processing_handshake()) return;
>> new L192: if (Thread::current() ==
>> thread->get_active_handshaker()) return;
>> I had to really think about logical equivalence here.
>> I think I've convinced myself that this is okay...
>>
>> src/hotspot/share/runtime/safepoint.cpp
>> No comments (see next file).
>>
>> src/hotspot/share/runtime/safepointMechanism.inline.hpp
>> The reason for the deletion of
>> SafepointMechanism::disarm_if_needed() is not obvious.
>>
>> src/hotspot/share/runtime/thread.hpp
>> No comments (nice encapsulation cleanup).
>>
>> src/hotspot/share/runtime/thread.cpp
>> No comments (nice encapsulation cleanup).
>>
>> test/hotspot/jtreg/runtime/handshake/HandshakeDirectTest.java
>> L107: workingThreads[0].join();
>> By only join()ing with the 0th worker thread, you won't
>> detect if any of the worker threads is stuck due to a bug.
>>
>> Dan
>>
>>
>> On 1/15/20 10:56 PM, Patricio Chilano wrote:
>>> Hi David,
>>>
>>> On 1/15/20 8:29 PM, David Holmes wrote:
>>>> Hi Patricio,
>>>>
>>>> On 16/01/2020 6:06 am, Patricio Chilano wrote:
>>>>> Hi David,
>>>>>
>>>>> Here is v2:
>>>>>
>>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8230594/v02/webrev/
>>>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8230594/v02/inc/webrev/
>>>>
>>>> src/hotspot/share/runtime/handshake.cpp
>>>>
>>>> why is it that execute_direct doesn't check for
>>>> uses_thread_local_poll() ?
>>> Sorry, I thought for a moment that thread-local handshakes for arm32
>>> was already implemented and all platforms where using it. I added
>>> that check back, and by testing that path I had to also add back the
>>> VMThread in the assert in walk_stack_and_revoke().
>>>
>>>> src/hotspot/share/runtime/handshake.hpp
>>>>
>>>> typo: wether -> whether
>>> Fixed.
>>>
>>> Here is v3:
>>> Full: http://cr.openjdk.java.net/~pchilanomate/8230594/v03/webrev/
>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8230594/v03/inc/webrev/
>>>
>>> Thanks!
>>>
>>> Patricio
>>>> Otherwise updates look fine.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Passed one round of t1-6, and running more rounds now.
>>>>>
>>>>> Thanks,
>>>>> Patricio
>>>>>
>>>>> On 1/14/20 7:57 PM, David Holmes wrote:
>>>>>> Hi Patricio,
>>>>>>
>>>>>> On 15/01/2020 5:17 am, Patricio Chilano wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> On 1/14/20 4:13 AM, David Holmes wrote:
>>>>>>>> Hi Patricio,
>>>>>>>>
>>>>>>>> Thanks for tackling this. I've taken an initial look through
>>>>>>>> trying to understand the general approach, but I don't claim to
>>>>>>>> understand all the subtle details, so there are some queries
>>>>>>>> below to help with my overall understanding.
>>>>>>>>
>>>>>>>> On 14/01/2020 2:25 am, Patricio Chilano wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> The following patch adds the ability to execute direct
>>>>>>>>> handshakes between JavaThreads without the VMThread
>>>>>>>>> intervention, and enables this functionality for biased
>>>>>>>>> locking revocations.
>>>>>>>>> The current handshake mechanism that uses the VMThread, either
>>>>>>>>> to handshake one JavaThread or all of them, is still the
>>>>>>>>> default unless you specify otherwise when calling
>>>>>>>>> Handshake::execute().
>>>>>>>>
>>>>>>>> Can I suggest that rather than using an overloaded:
>>>>>>>>
>>>>>>>> static bool execute(HandshakeClosure* hs_cl, JavaThread*
>>>>>>>> target, bool is_direct_handshake = false);
>>>>>>>>
>>>>>>>> you instead add an execute_direct method for clarity?
>>>>>>> Ok, added execute_direct() and removed overloading.
>>>>>>>
>>>>>>>>> In order to avoid adding additional overhead to this path that
>>>>>>>>> uses the VMThread (especially the one that handshakes all
>>>>>>>>> JavaThreads) I added a new HandshakeOperation pointer in the
>>>>>>>>> HandshakeState class, _operation_direct, to be used for the
>>>>>>>>> direct handshake cases only and whose access is serialized
>>>>>>>>> between JavaThreads by using a semaphore. Thus, one direct
>>>>>>>>> handshake will be allowed at any given time, and upon
>>>>>>>>> completion the semaphore will be signaled to allow the next
>>>>>>>>> handshaker if any to proceed. In this way the old _operation
>>>>>>>>> can still be used only by the VMThread without the need for
>>>>>>>>> synchronization to access it. The handshakee will now check if
>>>>>>>>> any of _operation or _operation_direct is set when checking
>>>>>>>>> for a pending handshake and will try to execute both in
>>>>>>>>> HandshakeState::process_self_inner(). The execution of the
>>>>>>>>> handshake’s ThreadClosure, either direct handshake or not, is
>>>>>>>>> still protected by a semaphore, which I renamed to
>>>>>>>>> _processing_sem.
>>>>>>>>> I converted the semaphore _done in HandshakeOperation to be
>>>>>>>>> just an atomic counter because of bug
>>>>>>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=12674 (which I
>>>>>>>>> actually hit once!). Since the semaphore could not be static
>>>>>>>>> anymore due to possibly having more than one
>>>>>>>>> HandshakeOperation at a time, the handshakee could try to
>>>>>>>>> access the nwaiters field of an already destroyed semaphore
>>>>>>>>> when signaling it. In any case nobody was waiting on that
>>>>>>>>> semaphore (we were not using kernel functionality), so just
>>>>>>>>> using an atomic counter seems more appropriate.
>>>>>>>>> In order to avoid issues due to disarming a JavaThread that
>>>>>>>>> should still be armed for a handshake or safepoint, each
>>>>>>>>> JavaThread will now always disarm its own polling page.
>>>>>>>>> I also added a new test, HandshakeDirectTest.java, which tries
>>>>>>>>> to stress the use of direct handshakes with revocations.
>>>>>>>>> In terms of performance, I measured no difference in the
>>>>>>>>> execution time of one individual handshake. The difference can
>>>>>>>>> be seen when several handshakes at a time are executed as
>>>>>>>>> expected. So for example on Linux running on an Intel Xeon
>>>>>>>>> 8167M cpu, test HandshakeDirectTest.java (which executes 50000
>>>>>>>>> handshakes between 32 threads) executes in around 340ms using
>>>>>>>>> direct handshakes and in around 5.6 seconds without it. For a
>>>>>>>>> modified version of that test that only executes 128
>>>>>>>>> handshakes between the 32 threads and avoids any
>>>>>>>>> suspend-resume, the test takes around 12ms with direct
>>>>>>>>> handshakes and 19ms without it.
>>>>>>>>> Tested with mach5, tiers1-6.
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8230594
>>>>>>>>> Webrev:
>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8230594/v01/webrev/
>>>>>>>>
>>>>>>>> src/hotspot/share/runtime/handshake.hpp
>>>>>>>>
>>>>>>>> 36 // A handshake closure is a callback that is executed for
>>>>>>>> each JavaThread
>>>>>>>> 37 // while that thread is in a safepoint safe state. The
>>>>>>>> callback is executed
>>>>>>>> 38 // either by the thread itself or by the VM thread while
>>>>>>>> keeping the thread
>>>>>>>> 39 // in a blocked state. A handshake can be performed with a
>>>>>>>> single
>>>>>>>> 40 // JavaThread as well.
>>>>>>>>
>>>>>>>> Does this comment block need updating for the direct case?
>>>>>>>>
>>>>>>>> 61 // the operation is only done by either VM thread on behalf
>>>>>>>> of the JavaThread
>>>>>>>> 62 // or the JavaThread itself.
>>>>>>>>
>>>>>>>> Again does this need an update?
>>>>>>> Yes, I forgot to update those comments in handshake.hpp. Fixed.
>>>>>>>
>>>>>>>
>>>>>>>> src/hotspot/share/runtime/handshake.hpp
>>>>>>>>
>>>>>>>> 41 class HandshakeOperation: public StackObj {
>>>>>>>>
>>>>>>>> Would it be clearer to have HandShakeOperation subclassed by
>>>>>>>> DirectHandShakeOperation, rather than it being a property of
>>>>>>>> the current op instance?
>>>>>>> But I would still need to access the is_direct field for
>>>>>>> non-direct handshakes in HandshakeState::try_process() anyways.
>>>>>>
>>>>>> Okay.
>>>>>>
>>>>>>>
>>>>>>>> 41 class HandshakeOperation: public StackObj {
>>>>>>>> 42 HandshakeClosure* _handshake_cl;
>>>>>>>> 43 int64_t _pending_threads;
>>>>>>>> ...
>>>>>>>> 53 void add_target_count(int count) {
>>>>>>>> Atomic::add(&_pending_threads, count); }
>>>>>>>>
>>>>>>>> Can you clarify the lifecycle of this StackObj please. You
>>>>>>>> obviously expose it to other threads - hence the atomic update
>>>>>>>> - but that means it needs to be guaranteed to be live when that
>>>>>>>> update occurs.
>>>>>>> The HandshakeOperation object is created in Handshake::execute()
>>>>>>> (or Handshake::execute_direct() now) by the Handshaker. The only
>>>>>>> other threads that could access this object are the VMThread(for
>>>>>>> non-direct case only) and the Handshakee(in both cases). Seeing
>>>>>>> that it's safe for the VMThread to decrement the counter is
>>>>>>> straightforward since the Handshaker will be waiting on the
>>>>>>> VMOperationRequest_lock waiting for the operation to finish
>>>>>>> (this is just as always has been).
>>>>>>> As for the Handshakee, we can see that the HandshakeOperation
>>>>>>> object will be alive as long as the VMThread(for the non-direct
>>>>>>> case) or Handshaker(for the direct case) keep seeing that the
>>>>>>> operation is not done by calling
>>>>>>> HandshakeOperation::is_completed(). Once that returns true, for
>>>>>>> the non-direct case the VMThread will finish the operation and
>>>>>>> wake up the Handshaker who will return from Handshake::execute()
>>>>>>> and the object will be destroyed, and for the direct case the
>>>>>>> Handshaker will just return from execute_direct() and the object
>>>>>>> will be destroyed. Since is_completed() will only return true
>>>>>>> when _pending_threads reaches zero, that means the decrement
>>>>>>> operation had to be safe. Just as an observation, for the
>>>>>>> HandshakeAllThreads operation, _pending_threads could go to zero
>>>>>>> and even negative before executing add_target_count(), but
>>>>>>> that's okay because the VMThread doesn't call is_completed()
>>>>>>> before that and the load of _pending_threads cannot float above
>>>>>>> add_target_count() since the atomic add operation provides a
>>>>>>> memory fence.
>>>>>>> And finally the Handshakee cannot try to execute an operation
>>>>>>> that has already being processed by the VMThread/Handshaker,
>>>>>>> because they clear it first and then signal the _processing_sem
>>>>>>> semaphore in HandshakeState::try_process(). That worked the same
>>>>>>> before this change.
>>>>>>
>>>>>> Thanks for the detailed explanation!
>>>>>>
>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> 349 HandshakeOperation* op =
>>>>>>>> Atomic::load_acquire(&_operation);
>>>>>>>> 350 if (op == NULL) {
>>>>>>>> 351 op = Atomic::load_acquire(&_operation_direct);
>>>>>>>>
>>>>>>>> This gives preference to the non-direct op. Is it possible for
>>>>>>>> the direct-op to be indefinitely delayed if there is a series
>>>>>>>> of non-direct ops?
>>>>>>> Yes, it's possible, although I think that's an unlikely
>>>>>>> scenario. That would mean the Handshakee would be stuck in that
>>>>>>> while loop with the VMThread constantly setting new operations
>>>>>>> in _operation. But I can modify the loop and make it try to
>>>>>>> execute both before going into the next interation, something
>>>>>>> like (more on load_acquire next):
>>>>>>>
>>>>>>> if (has_operation()) {
>>>>>>> HandleMark hm(_thread);
>>>>>>> CautiouslyPreserveExceptionMark pem(_thread);
>>>>>>> HandshakeOperation * op = _operation;
>>>>>>> if (op != NULL) {
>>>>>>> // Disarm before execute the operation
>>>>>>> clear_handshake(false);
>>>>>>> op->do_handshake(_thread);
>>>>>>> }
>>>>>>> op = _operation_direct;
>>>>>>> if (op != NULL) {
>>>>>>> // Disarm before execute the operation
>>>>>>> clear_handshake(true);
>>>>>>> op->do_handshake(_thread);
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> what do you think?
>>>>>>
>>>>>> That seems better to me.
>>>>>>
>>>>>>>
>>>>>>>> Also I don't see the release_stores that these load_acquire
>>>>>>>> would pair with. I'm assuming it should be here:
>>>>>>>>
>>>>>>>> 319 _operation = op;
>>>>>>>> 323 _operation_direct = op;
>>>>>>>>
>>>>>>>> But I would also have expected all necessary memory
>>>>>>>> synchronization to already be present via the semaphore
>>>>>>>> operations and/or the rest of the handshake mechanism. ??
>>>>>>> Yes, the release is in
>>>>>>> SafepointMechanism::arm_local_poll_release() after setting those
>>>>>>> fields. But it's true that those load_acquire are not needed
>>>>>>> since the semaphore already has acquire semantics. I actually
>>>>>>> wanted to do a normal load but ended up just copying how we were
>>>>>>> loading _operation. I''ll change them and retest.
>>>>>>
>>>>>> Okay.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -------
>>>>>>
>>>>>>>
>>>>>>>> 396 if ((!is_direct && _operation != NULL) || (is_direct &&
>>>>>>>> _operation_direct != NULL)){
>>>>>>>> 406 if ((!is_direct && _operation == NULL) || (is_direct &&
>>>>>>>> _operation_direct == NULL)){
>>>>>>>>
>>>>>>>> Can this not be abstracted back into a "has_operation" method
>>>>>>>> that takes a "direct" parameter? e.g
>>>>>>>>
>>>>>>>> bool has_operation() const { return _operation != NULL ||
>>>>>>>> _operation_direct != NULL; }
>>>>>>>> + bool has_specific_operation(bool direct) {
>>>>>>>> + return direct ? _operation_direct != NULL : _operation !=
>>>>>>>> NULL;
>>>>>>>> + }
>>>>>>> Done.
>>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> test/hotspot/jtreg/runtime/handshake/HandshakeDirectTest.java
>>>>>>>>
>>>>>>>> Please add:
>>>>>>>>
>>>>>>>> import java.util.concurrent.Semaphore;
>>>>>>>>
>>>>>>>> so you can just refer to Sempahore.
>>>>>>> Done.
>>>>>>>
>>>>>>>
>>>>>>>> Style nit: _working_threads
>>>>>>>>
>>>>>>>> Java style is to not use leading underscore and to use
>>>>>>>> camelCase for variables ie workingThreads.
>>>>>>> Right, I changed them to camelCase.
>>>>>>>
>>>>>>>
>>>>>>>> 44 static Thread _suspendresume_thread = new Thread();
>>>>>>>>
>>>>>>>> The above is dead code.
>>>>>>> Removed.
>>>>>>>
>>>>>>>
>>>>>>>> 53 if (_is_biased[me] == false) {
>>>>>>>>
>>>>>>>> Style nit: use "if (!_is_biased[me]) {"
>>>>>>> Changed.
>>>>>>>
>>>>>>>
>>>>>>>> 80 } catch(InterruptedException ie) {
>>>>>>>> 81 }
>>>>>>>>
>>>>>>>> I suggest inserting "throw new Error("Unexpected interrupt");"
>>>>>>>> for good measure.
>>>>>>>>
>>>>>>>> 111 _working_threads[i].suspend();
>>>>>>> Done.
>>>>>>>
>>>>>>>
>>>>>>>> Thread suspension has been deprecated-for-removal as of JDK 14
>>>>>>>> so could be gone in JDK 15 (but more likely 16). If the
>>>>>>>> suspend/resume is important for this test then you will need to
>>>>>>>> switch to using JVM TI suspend/resume; or else perhaps
>>>>>>>> introduce a WhiteBox method to do whatever you need in the VM.
>>>>>>> It's not really needed for this test. I just wanted to mixed
>>>>>>> suspend-resume with handshakes because I know there could be
>>>>>>> some buggy interactions between them. But since it has been
>>>>>>> deprecated then maybe there is no point in stressing handshakes
>>>>>>> with them and I can remove that part.
>>>>>>>
>>>>>>>
>>>>>>>> 123 // Wait until the desired number of direct handshakes is
>>>>>>>> reached
>>>>>>>> 124 while (_handshake_count.get() <
>>>>>>>> DIRECT_HANDSHAKES_MARK) {
>>>>>>>> 125 Thread.sleep(10); // sleep for 10ms
>>>>>>>> 126 }
>>>>>>>>
>>>>>>>> You could just do a join() on one of the worker threads.
>>>>>>> Right, didn't thought about that. Changed.
>>>>>>>
>>>>>>>
>>>>>>> Thanks for looking at this David! I'll retest with the above
>>>>>>> changes in the meantime.
>>>>>>>
>>>>>>>
>>>>>>> Patricio
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>> Patricio
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
More information about the hotspot-runtime-dev
mailing list