RFR: 8214278: Cleanup process_completed_threshold and related state

[Stefan Johansson]

Hi Kim,

On 2018-11-26 13:43, Thomas Schatzl wrote:
> Hi Kim,
>
> On Sun, 2018-11-25 at 20:44 -0500, Kim Barrett wrote:
>> This change addresses two CRs:
>> 8214278: Cleanup process_completed_threshold and related state
>> 8156696: Simplify PtrQueueSet initialization
>>
>> This change is based on the change for 8214202:
>> DirtyCardQueueSet::get_completed_buffer should not clear
>> _process_completed, which is already out for review.
>>
>> Please review this change to some of the data members in PtrQueueSet,
>> to simplify their semantics and usage.  The affected data members are
>>
>> _process_completed_threshold
>> _max_completed_queue
>> _completed_queue_padding
>>
>> The types of all of them are now size_t (some were int).  There is no
>> longer any special treatment of zero (and negative is no longer
>> possible).  The only special treatment now required is the
>> possibility of overflow when adding max completed and padding.
>>
>> The members are now initialized to extreme values (either 0 or max,
>> as appropriate), with PtrQueueSet::initialize no longer taking any
>> associated arguments.  Instead, through proper choice of the initial
>> values, some require no further initialization, and the rest are
>> updated via the associated setters.  (This addresses JDK-8156696.)
>>
>> Added missing const qualifiers for associated reader functions.
>>
>> Finally, the following renamings were made, for improved naming
>> consistency:
>>
>> process_completed_threshold => process_completed_buffers_threshold
>> max_completed_queue => max_completed_buffers
>> completed_queue_padding => completed_buffers_padding
>>
>> (There are still some naming inconsistencies around "completed
>> buffers", which I'm planning to address in future RFEs.)
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8214278
>> https://bugs.openjdk.java.net/browse/JDK-8156696
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8214278/open.00/
Looks good. I agree with Thomas about the constants, I would also prefer 
camel case.

Cheers,
Stefan

>>
>> Testing:
>> mach5 tier1-5
>>
> Thanks for doing this cleanup.
>
> Some minor nits:
>
> - in DirtyCardQueueSet::concatenate_logs() a SizeTFlagSetting could be
> used to do the temporarily-change-value dance. (I know, the class is
> very specifically named).
>
> - not sure if static consts like
> _process_completed_buffers_threshold_never should be named like a
> member. I think at some point we agreed on (but did not write down) to
> use CamelCase for those.
>
> Just mentioning: another option than doing the overflow check in
> PtrQueueSet::process_or_enqueue_complete_buffer could be limiting
> _completed_buffers_padding to something like INT_MAX as we already do
> for _max_completed_buffers.
> But I think the existing check is less noise than changing the padding
> code.
>
> Thanks,
>    Thomas
>
>