RFR: 8214201: Make PtrQueueSet completed buffer list private

[Kim Barrett]

> On Dec 3, 2018, at 10:12 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Sun, 2018-12-02 at 01:44 -0500, Kim Barrett wrote:
>> Please review this refactoring of PtrQueueSet and its derivations to
>> make the data members for the completed buffer list private (rather
>> than protected), and clean up the associated API.  This also merges
>> some otherwise nearly duplicated code from DirtyCardQueueSet and
>> SATBMarkQueueSet into PtrQueueSet.
>> 
>> Moved DirtyCardQueueSet::get_completed_buffer() to PtrQueueSet, and
>> changed SATBMarkQueueSet to use it too, rather than having its own
>> inline near copy.  Note that get_completed_buffer clears the
>> _process_completed_buffers flag when the list becomes empty, as
>> needed for the SATB case; the DCQS case doesn't care.  Also take care
>> to NULL out the next field of the returned buffer; currently no code
>> cares, but in the future we might add asserts when enqueuing or
>> freeing.
> 
> I think this is okay.

Thanks.

>> Added PtrQueueSet::abandon_completed_buffers().  Changed SATB and DC
>> queue sets to use it rather than duplicate code.
>> 
>> Made some more naming updates for consistent nomenclature.  We now
>> consistently use "completed_buffer" rather than "complete_buffer" or
>> "completed".
>> - enqueue_complete_buffer() => enqueue_completed_buffer()
>> - _process_completed => _process_completed_buffers
>> - set_process_completed() => set_process_completed_buffers()
>> (Note: Already had process_completed_buffers()).
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8214201
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8214201/open.00/
>> 
> 
> In prtQueue.hpp:278, could you fix the comment with the "XXX" while we
> are in the area? I think even just deleting that line is an
> improvement…

Good point.  Deleted.

> Maybe move PtrQueueSet::_notify_when_complete and _all_active after the
> _max_completed_buffers and the _completed_buffers_padding; the latter
> are directly related to buffer completion started above the
> _all_active, while the others seem to be more related to thread
> management. But it is fine to keep it as is too.

_notify_when_complete is related to _process_completed_buffers_threshold,
in that notification is done when the notify flag is true and the
number of buffers exceeds the threshold.

But _all_active is indeed completely unrelated to the others. And if
I'm not misremembering, the whole active/inactive distinction is only
relevant to the SATBMarkQueue(Set), with DirtyCardQueue(Set) just
always being "active", so maybe it shouldn't be at the PtrQueue(Set)
layer at all.  But doing anything about that would be for some future
RFE.  (Note that I have more changes coming in this area, some at
least partially developed.  But moving these declarations around a bit
here looks pretty harmless to that work.)

> Looks good. No re-review for any of these suggestions needed.

Thanks for reviewing.

> 
> Thanks,
>  Thomas