RFR: 8234796: Refactor Handshake::execute to take a HandshakeOperation

[Per Liden]

Hi Robbin,

On 11/26/19 2:07 PM, Robbin Ehn wrote:
> Hi all, please review.
> 
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8234796
> Code:
> http://cr.openjdk.java.net/~rehn/8234796/full/webrev/
> 
> The handshake code needs more information about the handshake operation.
> We change type from ThreadClosure to HandshakeOperation in 
> Handshake::execute.
> This enables us to add more details to the HandshakeOperation as needed 
> going forward.

I kind of think HandshakeOperation is exposing a too wide API here. I'm 
thinking the following things doesn't quite belong in there, but is more 
Handshake internal stuff.

   void do_handshake(JavaThread* thread);
   bool thread_has_completed() { return _done.trywait(); }
   bool executed() const { return _executed; }

#ifdef ASSERT
   void check_state() {
     assert(!_done.trywait(), "Must be zero");
   }
#endif

How about you just expose a closure that inherits from ThreadClosure, 
but also takes a name? Like this:

class HandshakeClosure : public ThreadClosure {
private:
   const char* const _name;

public:
   HandshakeClosure(const char* name) :
       _name(name) {}

   const char* name() const {
     return _name;
   }

   virtual void do_thread(Thread* thread) = 0;
};

That way we expose a narrower API, and it also helps avoid the need for 
HandshakeOperation -> ThreadClosure wrappers. The other stuff can stay 
internal in a wrapping HandshakeThreadOperation like it did before.

What do you think?

/Per

> 
> Tested t1 and t1-3 together with the logging improvements in 8234742.
> 
> It was requested that "HandshakeOperation()" would take the name instead 
> having "virtual const char* name();". Which is in this patch.
> 
> Thanks, Robbin