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