RFR: 8266557: assert(SafepointMechanism::local_poll_armed(_handshakee)) failed: Must be [v3]

[Daniel D.Daugherty]

On Wed, 2 Jun 2021 09:20:11 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> src/hotspot/share/runtime/handshake.cpp line 429:
>> 
>>> 427:   assert(_handshakee == Thread::current(), "Must be called by self");
>>> 428:   assert(_lock.owned_by_self(), "Lock must be held");
>>> 429:   return _queue.peek();
>> 
>> `get_op_for_self()` name does not match the `peek()` operation.
>> The new name should be `peek_op_for_self()`.
>
> The method retrieves the next operation to be executed, hence 'get'.
> That the method uses a queue which provides a pop() and peek() is an implementation, there is no value for the user of  get_op_for_self() to know how this is done?

Agreed there is no value in that distinction. I was having trouble getting my head
wrapped around what the code was trying to do.

>> src/hotspot/share/runtime/handshake.cpp line 445:
>> 
>>> 443:   assert(_handshakee != Thread::current(), "Must not be called by self");
>>> 444:   assert(_lock.owned_by_self(), "Lock must be held");
>>> 445:   return _queue.peek(non_self_queue_filter);
>> 
>> `get_op()` name does not match the peek() operation.
>> The new name should be `peek_op()`.
>
> The method retrieves the next operation to be executed, hence 'get'.
> That the method uses a queue which provides a pop() and peek() is an implementation, there is no value for the user of  get_op_for_self() to know how this is done?

Agreed.

>> src/hotspot/share/runtime/handshake.cpp line 452:
>> 
>>> 450:   MatchOp mo(op);
>>> 451:   HandshakeOperation* ret = _queue.pop(mo);
>>> 452:   assert(ret == op, "OP missing from queue");
>> 
>> Perhaps this for the assert() mesg:
>> `  assert(ret == op, "popped op must match requested op.");
>> 
>> Also, I would name this one `pop_op()` instead of `remove_op()`.
>
> Changed assert.
> 
> In the handshake code we need two operation get operation and remove operation.
> They happen to be implemented with peek and pop.
> So from the user perspective he wants to remove an operation and thus uses the remove method.
> Since we do not want any return from this method, renaming it pop and not return anything is just confusing, isn't it?

Agreed.

>> src/hotspot/share/runtime/handshake.cpp line 483:
>> 
>>> 481:         HandleMark hm(_handshakee);
>>> 482:         PreserveExceptionMark pem(_handshakee);
>>> 483:         op->do_handshake(_handshakee); // acquire, op removed after
>> 
>> Why `acquire`? Perhaps:
>> `// peek and handshake with op; op removed after`
>
> It's mean do_handshake provide acquire thus op is removed after.

Ok.

>> src/hotspot/share/runtime/handshake.cpp line 489:
>> 
>>> 487:         // The destructor ~PreserveExceptionMark touches the exception oop so it must not be executed,
>>> 488:         // since a safepoint may be in-progress when returning from the async handshake.
>>> 489:         op->do_handshake(_handshakee); // acquire, op removed after
>> 
>> Why acquire? Perhaps:
>> // peek and handshake with op; op removed after
>
> It's mean do_handshake provide acquire thus op is removed after.

Ok.

>> src/hotspot/share/runtime/handshake.cpp line 583:
>> 
>>> 581: 
>>> 582:   set_active_handshaker(current_thread);
>>> 583:   op->do_handshake(_handshakee); // acquire, op removed after
>> 
>> Why acquire? Perhaps:
>> // peek and handshake with op; op removed after
>
> It's mean do_handshake provide acquire thus op is removed after.

Ok.

>> src/hotspot/share/utilities/filterQueue.inline.hpp line 127:
>> 
>>> 125:   do {
>>> 126:     if (match_func(cur->_data)) {
>>> 127:       match = cur;
>> 
>> Hmmm.... I was expecting a break after this match.
>> Is there a reason to continue the loop?
>> 
>> But now I'm realizing that I'm confused since this is called `peek()`, but
>> it's not really peeking at the head of the queue. It is searching the entire
>> queue for a match.
>
> It is a FIFO queue, which provides inserts at the end (push) and retrieves at the front of the queue (pop/peek).
> Push inserts at tail and pop/peek looks at head. (maybe these should be named differently to reflect this better, but that is another PR :) )
> 
> To provide lock-free inserts the queue only have a tail pointer.
> This enabled us to do concurrent inserts with other inserts and/or another serialized operation, such as peek.
> To get to the front of the queue we must always walk until the end.
> 
> This means peek is peeking at the head (with a filter).
> Conceptually we generate a queue from that filter and from that virtual queue we peek at it's head.
> 
> Tail->A1->A2->A3->A4->A5
> Filter = even
> Virtual tail->A2->A4
> Virtual head = A4
> 
> To get to A4 we walk and apply the filter, the last match will be the head we are looking to peek at.

So it's not just find _a_ match. It is find the _last_ match. OK.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3973