On Mon, 28 Sep 2020 05:28:43 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Robbin Ehn has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add constructor and comment. Previous renames caused confusing, improved
>> names once more and moved non-public to private
>
> src/hotspot/share/runtime/handshake.cpp line 51:
>
>> 49: private:
>> 50: // Must use AsyncHandshakeOperation when using AsyncHandshakeClosure.
>> 51: HandshakeOperation(AsyncHandshakeClosure* cl, JavaThread* target,
>> jlong start_ns) {};
>
> I'm really not understanding the way you have implemented the guard I
> requested. How does declaring the private
> constructor prevent a call to HandShakeOperation(someAsync, target) ?
Sorry my bad, fixed.
> src/hotspot/share/runtime/handshake.cpp line 202:
>
>> 200: }
>> 201:
>> 202: static void log_handshake_info(jlong start_time_ns, const char* name,
>> int targets, int non_self_executed, const
>> char* extra = NULL) {
>
> The name "non_self_executed" is much clearer - thanks. But this now
> highlights that the log message itself doesn't make
> complete sense as it refers to "requesting thread" when it could be a range
> of possible threads not just a single
> requesting thread.
This log line is done by the requesting thread of the specific "Handshake "%s".
The log line only prints executed handshake operations emitted by the
requesting thread.
With changes on how logs from @pchilano and name changes this is getting
confusing for me also.
I renamed non_self_executed to emitted_handshakes_executed as the local
variable passed into this function.
To summarize the log line prints how many of the handshake operation emitted by
'this' handshake request was done by
the requesting thread (or by VM Thread on behalf of the requesting thread when
doing handshake all). This value can
thus only be 0 or 1 if not executed by VM thread in a handshake all.
Operations not done by requesting thread was either done cooperatively or by
targets self.
> src/hotspot/share/runtime/handshake.cpp line 239:
>
>> 237: // Keeps count on how many of own emitted handshakes
>> 238: // this thread execute.
>> 239: int emitted_handshakes_executed = 0;
>
> I suggest:
> // Keeps count of how many handshakes were actually executed
> // by this thread.
> int handshakes_executed = 0;
We only count those we emitted.
I use to count all, but @pchilano thought it was confusing that we could log
more executed than emitted.
So now it only counts executed and emitted.
> src/hotspot/share/runtime/handshake.cpp line 326:
>
>> 324: // Keeps count on how many of own emitted handshakes
>> 325: // this thread execute.
>> 326: int emitted_handshakes_executed = 0;
>
> See previous comment.
Dito :)
> src/hotspot/share/runtime/handshake.cpp line 389:
>
>> 387: };
>> 388:
>> 389: static bool non_self_queue_filter(HandshakeOperation* op) {
>
> The name "non_self_queue_filter" is really awkward - sorry. But I think we're
> going to have to revisit the way
> filtering is named and done once we try to generalise it anyway, so I'll let
> this pass.
Sure
> src/hotspot/share/runtime/handshake.hpp line 79:
>
>> 77: // Provides mutual exclusion to this state and queue.
>> 78: Mutex _lock;
>> 79: // Set to the thread executing the handshake operation during the
>> execution.
>
> s/the execution/its execution/
>
> Or just delete "during the execution"
Fixed
> src/hotspot/share/runtime/handshake.hpp line 87:
>
>> 85: void process_self_inner();
>> 86:
>> 87: bool have_non_self_executable_operation();
>
> API naming consistency is still a problem here. We have:
> - pop_for_self()
> - pop()
>
> but
> - has_operation() // for use by self/handshakee
> - have_non_self_executable_operation()
>
> why not:
> - has_operation_for_self()
> - has_operation()
>
> ?
has_operation() is used by other threads than self, since has_operation() is
lock free.
Since 99% of the time the queue is empty, we get the correct answers without
needing to lock the handshake state lock.
This is signification for handshake all, since we keep revisiting threads even
when they have completed their
opretaion. It is thus very confusing having another thread calling
has_operation_for_self().
> src/hotspot/share/runtime/handshake.hpp line 100:
>
>> 98: }
>> 99:
>> 100: // Both _queue and _lock must be check. If a thread have seen this
>> _handshakee
>
> s/check/checked/
> s/have/has/
Fixed
> src/hotspot/share/runtime/handshake.hpp line 103:
>
>> 101: // as safe it will execute all possible handshake operations in a
>> loop while
>> 102: // holding _lock. We use lock free addition to the queue, which means
>> it is
>> 103: // possible to the queue to be seen as empty by _handshakee but as
>> non-empty
>
> s/to/for/
Fixed
> src/hotspot/share/runtime/handshake.hpp line 105:
>
>> 103: // possible to the queue to be seen as empty by _handshakee but as
>> non-empty
>> 104: // by the thread executing in the loop. To avoid the _handshakee
>> eliding
>> 105: // stopping while handshake operations are being executed, the
>> _handshakee
>
> s/eliding stopping/continuing/ ?
Sure
> src/hotspot/share/runtime/handshake.hpp line 106:
>
>> 104: // by the thread executing in the loop. To avoid the _handshakee
>> eliding
>> 105: // stopping while handshake operations are being executed, the
>> _handshakee
>> 106: // must take slow if _lock is held and make sure the queue is empty
>> otherwise
>
> Unclear what "slow" refers to - "take the slow path"? But what path is that?
Fixed
> src/hotspot/share/runtime/handshake.hpp line 107:
>
>> 105: // stopping while handshake operations are being executed, the
>> _handshakee
>> 106: // must take slow if _lock is held and make sure the queue is empty
>> otherwise
>> 107: // try process it.
>
> s/try/try to/
>
> but I'm not sure how this follows the "otherwise" in this sentence.
Fixed/removed
> src/hotspot/share/utilities/filterQueue.hpp line 32:
>
>> 30:
>> 31: // The FilterQueue is FIFO with the ability to skip over queued items.
>> 32: // The skipping is controlled by using a filter when poping.
>
> s/poping/popping/
Fixed
> src/hotspot/share/utilities/filterQueue.hpp line 33:
>
>> 31: // The FilterQueue is FIFO with the ability to skip over queued items.
>> 32: // The skipping is controlled by using a filter when poping.
>> 33: // It also supports lock free pushes, while poping (including contain())
>
> s/poping/popping/
> s/contain()/contains()/
Fixed
> src/hotspot/share/utilities/filterQueue.hpp line 64:
>
>> 62: // Applies the match_func to the items in the queue until match_func
>> returns
>> 63: // true and then return false, or there is no more items and then
>> returns
>> 64: // false. Any pushed item while executing may or may not have
>> match_func
>
> s/pushed item/item pushed/
>
> I know what you are trying to say about concurrent pushes but it sounds too
> non-deterministic - any concurrent push
> that happens before contains() reaches the end of the queue will have the
> match_func applied. So in that sense "while
> executing" only applies to pushes that will be seen; any push not seen had to
> have happened after execution was
> complete.
We insert on first ptr and we also walk from first ptr. (This is tail of the
queue)
Since contains() never restart and we load first on directly on entry, no new
pushes will be seen by contains().
pop() on the other hand may re-start due to failed CAS, thus all pushes up till
this failed CAS will be seen.
This happens if first ptr (tail) is the items selected for popping.
A new push will change the first ptr (tail), the re-start to be able to
unlinked the match.
With a deterministic match_func this newly pushed item will never be pop, since
we have an older matching item.
(otherwise we would never tried to CAS)
> src/hotspot/share/utilities/filterQueue.hpp line 63:
>
>> 61:
>> 62: // Applies the match_func to the items in the queue until match_func
>> returns
>> 63: // true and then return false, or there is no more items and then
>> returns
>
> Surely "true and then returns true"?
> s/there is/there are/
Fixed
> src/hotspot/share/utilities/filterQueue.hpp line 66:
>
>> 64: // false. Any pushed item while executing may or may not have
>> match_func
>> 65: // applied. The method is not re-entrant and must be executed mutually
>> 66: // exclusive other contains and pops calls.
>
> // exclusive to other contains() and pop() calls.
Fixed
> src/hotspot/share/utilities/filterQueue.hpp line 77:
>
>> 75:
>> 76: // Applies the match_func to all items in the queue returns the item
>> which
>> 77: // match_func return true for and was inserted first. Any pushed item
>> while
>
> // Applies the match_func to each item in the queue, in order of insertion,
> and
> // returns the first item for which match_func returns true. Returns false if
> there are
> // no matches or the queue is empty.
Some offline discussion, fixed.
-------------
PR: https://git.openjdk.java.net/jdk/pull/151
