RFR 8230594: Allow direct handshakes without VMThread intervention
David Holmes
david.holmes at oracle.com
Sat Jan 18 00:24:26 UTC 2020
On 18/01/2020 10:02 am, Patricio Chilano wrote:
> Hi David,
>
> On 1/17/20 8:17 PM, David Holmes wrote:
>> Hi Patricio,
>>
>> Your v4 reintroduced the suspend/resume in the test! ??
> Yes, Coleen thought it was useful to keep it as a way to stress the
> handshake code, and since suspend-resume is still there I think maybe it
> doesn't hurt to add it to the test. We could remove it once
> suspend-resume is actually removed from Hotspot, what do you think?
The core-libs folk could remove support tomorrow (unlikely but
possible). So this just adds something that has to be fixed when they do
that.
But seems I'm out voted so it stays.
David
> Patricio
>> Cheers,
>> David
>>
>> On 18/01/2020 9:06 am, Patricio Chilano wrote:
>>> Hi Dan,
>>>
>>> On 1/16/20 7:23 PM, 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.
>>> Done.
>>>
>>>> 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".)
>>> Done.
>>>
>>>> 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.
>>> Done.
>>>
>>>> L42: // or not, by the JavaThread that requested the handshake or
>>>> the VMThread
>>>> Can handshakes be requested by non-JavaThreads?
>>> I wrote the code with the assumption that direct handshakes will be
>>> between JavaThreads only, since at least for now this is going to be
>>> used only for biased locking revocations. Changing it to allow
>>> non-JavaThreads to request direct handshakes should be
>>> straightforward though. We just wouldn't need to call
>>> SafepointMechanism::should_block() to check for pending handshakes
>>> and we wouldn't need to wait with safepoint check in set_operation().
>>> I wouldn't be able to test it though since we don't have those kind
>>> of direct handshakes yet. Maybe we can change it later when we have a
>>> real case?
>>>
>>>> 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.
>>> Done. If you are okay with the previous point I changed the last
>>> "thread" to "JavaThread".
>>>
>>>> L65: // the JavaThread or the JavaThread itself.
>>>> nit: s/or the/or by the target/
>>> Done.
>>>
>>>> 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:
>>> Done.
>>>
>>>> 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.
>>> Done.
>>>
>>>> L286: HandshakeOperation op(thread_cl, true);
>>>> Please prefix literal booleans with /*is_direct*/ so we know
>>>> what
>>>> the parameter is.
>>> Done.
>>>
>>>> L341: assert(Thread::current() == _thread, "should call from
>>>> thread");
>>>> This should probably be JavaThread::current().
>>> I read the discussion you had with David and I think you both agree
>>> on keeping Thread::current() so I didn't change it.
>>>
>>>> L356: // Disarm before execute the operation
>>>> L362: // Disarm before execute the operation
>>>> nit: s/execute/executing/
>>> Done.
>>>
>>>> L357: clear_handshake(false);
>>>> L363: clear_handshake(true);
>>>> Please prefix literal booleans with /*is_direct*/ so we know
>>>> what
>>>> the parameter is.
>>> Done.
>>>
>>>> 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)?
>>> Because we only successfully decremented the semaphore if
>>> claim_handshake() returns true. In all the previous "return false"
>>> cases we either did not try to decrement it or we failed to do so.
>>>
>>>> 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".
>>> Done.
>>>
>>>> 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...
>>> Without this patch, only the VMThread could be processing a handshake
>>> closure for a given JavaThread. But now it can also be executed by a
>>> JavaThread. Hence I changed the code to check whether the current
>>> thread is the one executing the handshake closure on behalf of the
>>> JavaThread passed as a parameter to
>>> assert_locked_or_safepoint_or_handshake().
>>>
>>>> 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.
>>> Before, only the VMThread would arm a JavaThread, either for a
>>> safepoint or a handshake. The disarm could be made by either of them.
>>> The disarm by the VMThread was always safe, since it would do it
>>> after the operation was completed. For the JavaThread not quite. To
>>> avoid disarming a still pending safepoint/handshake the JavaThread
>>> immediately rechecks if there is a pending operation after doing the
>>> disarm, and in case there is, it arms itself again (that code is in
>>> SafepointMechanism::block_if_requested_slow()).
>>> If we now allow JavaThreads doing direct handshakes or the VMThread
>>> to disarm the polling page of a given JavaThread then we would need
>>> to protect access to the polling page with some synchronization
>>> primitive to avoid disarming a current pending operation (VMThread
>>> disarming when there is still a pending direct handshake, or
>>> JavaThread disarming when there is still a pending
>>> safepoint/handshake). To avoid adding additional overhead in
>>> accessing the polling page we can make each JavaThread disarm itself.
>>>
>>>> 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.
>>> Right, I changed it and now I do join() with all the workers.
>>>
>>> Here is v4 addressing also Coleen's comments. Hope I didn't miss
>>> anything:
>>>
>>> Full: http://cr.openjdk.java.net/~pchilanomate/8230594/v04/webrev/
>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8230594/v04/inc/webrev/
>>>
>>> Thanks!
>>>
>>> Patricio
>>>> 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