RFR: 8237143: Eliminate DirtyCardQ_cbl_mon

[Thomas Schatzl]

Hi Kim,

On 16.01.20 09:51, Kim Barrett wrote:
> Please review this change to eliminate the DirtyCardQ_cbl_mon.  This
> is one of the two remaining super-special "access" ranked mutexes.
> (The other is the Shared_DirtyCardQ_lock, whose elimination is covered
> by JDK-8221360.)
> 
> There are three main parts to this change.
> 
> (1) Replace the under-a-lock FIFO queue in G1DirtyCardQueueSet with a
> lock-free FIFO queue.
> 
> (2) Replace the use of a HotSpot monitor for signaling activation of
> concurrent refinement threads with a semaphore-based solution.
> 
> (3) Handle pausing of buffer refinement in the middle of a buffer in
> order to handle a pending safepoint request.  This can no longer just
> push the partially processed buffer back onto the queue, due to ABA
> problems now that the buffer is lock-free.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8237143
> 
> Webrev:
> https://cr.openjdk.java.net/~kbarrett/8237143/open.00/
> 
> Testing:
> mach5 tier1-5
> Normal performance testing showed no significant change.
> specjbb2015 on a very big machine showed a 3.5% average critical-jOPS
> improvement, though not statistically significant; removing contention
> for that lock by many hardware threads may be a little bit noticeable.
> 
> 

   initial comments only, and so far only about comments :( The code 
itself looks good to me, but I want to look over it again.

- noticed at least in maybe_deactivate, the description of the method 
and parameter and return value documentation is in the cpp file at the 
definition. I would really prefer this information in the hpp file for 
easier reference.

- documentation about the inner workings of the class should imho be put 
in the hpp file too (in the class documentation). Same as documentation 
about member variables in seemingly random places now.

This makes the code unusually hard to read and reference for me - i.e. 
you can easily flip to the definition (hpp file) using a click (or 
accelerator key) when on the identifier to see how stuff is supposed to 
be used (or what it is used for) than literally grepping through the cpp 
file (and these blocks of documentation even reference each other).

Also, IDEs show comments on methods in the hpp as pop-ups too.

E.g. the documentation of _notifier and _should_notify in 
g1ConcurrentRefineThread.hpp:51, or the description of 
G1DirtCardQueueSet::_concurrency in g1DirtyCardQueue.cpp:117 (followed 
by description of (Non)ConcurrentVerifier, or the huge description of 
_completed_buffers_{head,tail} and others in G1DirtyCardQueue.cpp:163, 
and the description of refinement processing in g1DirtyCardQueue.cpp:301.

I mean, if a user tries to get an overview on how the class works or 
what some members do I would argue that you would first reference the 
hpp file - and the only new comment there right now is some reasoning 
for the padding of the new members without any comment about the purpose 
of these.
Maybe some of the comments should be split into more generic parts, and 
very specific implementation details (which can stay in the cpp file 
where they are needed - these seem sufficient now?).

- just a random note: I wasn't really happy with the name of 
G1ConcurrentRefineThread::_should_notify, but several attempts at 
renaming failed for me. It seems rather generic.

- some comments on the long comment about the FIFO queue handling.

// _completed_buffers_{head,tail} and _num_cards provide a lock-free
// FIFO of buffers, linked through their next() fields.

Not sure about whether _num_cards provides anything about the FIFO (it's 
not even the number of buffers), it seems to be solely counting the 
cards held in the FIFO. Which is fine to mention, but not necessarily here.

Also I would put that description of _completed_buffers to the variables 
in the hpp files.

// The key idea to make this work is that pop (get_completed_buffer)
// never returns an element of the queue if it is the only accessible
// element,

If I understand this correctly, maybe "if there is only one buffer in 
the FIFO" is easier to understand than "only accessible element". (or 
define "accessible element").

// e.g.  its "next" value is NULL.  It is expected that there

s/e.g./i.e.?

The code seems to unnecessarily use the NULL_buffer constant. Maybe use 
it here too. Overall I am not sure about the usefulness of using 
NULL_buffer in the code. The NULL value in Hotspot code is generally 
accepted as a special value, and the name "NULL_buffer" does not seem to 
add any information.

// will be a later push/append that will make that element available to

I would prefer if the documentation would be consistent with the 
nomenclature in the code, i.e. use either append/enqueue/get throughout 
or some variants of push/pop (there is no method that starts with either 
push or pop anywhere). While I understand that you'd typically use 
push/pop on a FIFO I would prefer to either rename the methods or drop 
the use of push/pop in this documentation. Additional identical 
terminology seems confusing. At least it takes time to find out what is 
exactly what.

What would be nice in this context would be that G1DirtyCardQueueSet is 
implemented as a FIFO of buffers, and that append/enqueue/get* methods 
are the usual operations. (in the hpp file, in the class description)

// a future pop, or there will eventually be a complete transfer
// (take_all_completed_buffers).
//
// An append operation atomically exchanges the new tail with the queue
// tail. It then sets the "next" value of the old tail to the head of
// the list being appended.  (It is an invariant that the old tail's
// "next" value is NULL.)

Maybe put this invariant somewhere more prominent in the text, not as 
side note.

// But if the old tail is NULL then the queue was
// empty.  In this case the head of the list being appended is instead
// stored in the queue head (which must be NULL).

I would mention the invariant that if old tail is NULL then head must be 
NULL too right next to the "old tail is NULL" sentence.

// A push operation is just a degenerate append, where the buffer being
// pushed is both the head and the tail of the list being appended.

Defining a push operation does not seem to help at all, because the 
documentation always mentions the pair push/append anyway (and there is 
no explicit "push" method in the code). I would suggest to delete this 
paragraph.

//
// This means there is a period between the exchange and the old tail
// update where the queue sequence is split into two parts, the list
// from the queue head to the old tail, and the list being appended.  If
// there are concurrent push/append operations, each may introduce
// another such segment.  But they all eventually get resolved by their
// respective updates of their old tail's "next" value.
//
// pop gets the queue head as the candidate result (returning NULL if
// the queue head was NULL), and then gets that result node's "next"
// value.  If that "next" value is NULL and the queue head hasn't
// changed, then there is only one element in the (accessible) list.  We

It would be nice to define the "accessible" list somewhere explicitly - 
or drop that property because it seems to be the standard "elements 
within the current head and tail" anyway.

// can't return that element, because it may be the old tail of a
// concurrent push/append.  So return NULL in this case.  Otherwise,
// attempt to cmpxchg that "next" value into the queue head, retrying
// the whole operation if that fails. This is the "usual" lock-free pop
// from head of slist, with the additional restriction on taking the

slist? ;)

// last element.

s/taking/popping (or "get"-ing) the last element.

// In order to address the ABA problem for pop, a pop operation protects
// its access to the head of the list with a GlobalCounter critical
// section. This works with the buffer allocator's use of GlobalCounter
// synchronization to prevent ABA from arising in the normal buffer
// usage cycle.  The paused buffer handling prevents another ABA source
// (see record_paused_buffer and enqueue_previous_paused_buffers).

- g1DirtyCardQueue.cpp: s/"// Unreachable"/ShouldNotReachHere(); (or 
just delete)

Thanks,
   Thomas