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

[Robbin Ehn]

On Tue, 1 Jun 2021 20:52:29 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>> 
>>  - Merge branch 'master' into handshakee
>>  - Small update
>>  - Merge branch 'master' into handshakee
>>  - Fix
>
> 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?

> 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.

> 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.

> 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.

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

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