RFR 8230594: Allow direct handshakes without VMThread intervention
David Holmes
david.holmes at oracle.com
Tue Jan 14 07:13:51 UTC 2020
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?
> 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?
---
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?
--
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.
--
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?
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. ??
--
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;
+ }
---
test/hotspot/jtreg/runtime/handshake/HandshakeDirectTest.java
Please add:
import java.util.concurrent.Semaphore;
so you can just refer to Sempahore.
Style nit: _working_threads
Java style is to not use leading underscore and to use camelCase for
variables ie workingThreads.
44 static Thread _suspendresume_thread = new Thread();
The above is dead code.
53 if (_is_biased[me] == false) {
Style nit: use "if (!_is_biased[me]) {"
80 } catch(InterruptedException ie) {
81 }
I suggest inserting "throw new Error("Unexpected interrupt");" for good
measure.
111 _working_threads[i].suspend();
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.
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.
Thanks,
David
-----
> Thanks!
> Patricio
>
More information about the hotspot-runtime-dev
mailing list