RFR (S) JDK-8139651 JDK-8139651, ConcurrentG1Refine uses ints for many of its members that should be unsigned types

[Tom Benson]

Hi Joe,
In g1_globals.hpp, you changed G1ConcRefinementServiceIntervalMillis to 
size_t, along with the zone sizes and threshold.  I think that one 
should be uintx, since it's definitely a count, not size/length.

Also, I notice you didn't change the max_jint I mentioned in 
dirtyCardQueue.cpp:

329   _max_completed_queue = max_jint;

Was that by design or by accident?  8^)

Aside from that, looks OK to me.
Tom


On 2/19/2016 11:05 AM, Joseph Provino wrote:
> New webrev is here: 
> http://cr.openjdk.java.net/~jprovino/8139651/webrev.01
>
> thanks.
>
> joe
>
> On 2/18/2016 1:44 PM, Joseph Provino wrote:
>>
>>
>> On 2/12/2016 6:49 PM, Kim Barrett wrote:
>>>> On Feb 11, 2016, at 6:58 PM, Joseph Provino 
>>>> <joseph.provino at oracle.com> wrote:
>>>>
>>>> Please review this small change from "int" to "size_t".
>>>>
>>>> CR:  https://bugs.openjdk.java.net/browse/JDK-8139651
>>>>
>>>> webrev: http://cr.openjdk.java.net/~jprovino/8139651/webrev.00
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> Copyrights need updating.
>> Okay.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> I think the types of the command line options should be changed to be
>>> consistent with their associated concurrent refinement values, e.g.
>>> G1ConcRefinementGreenZone should have the same type as
>>> ConcurrentG1Refine::_green_zone and the associated accessor functions.
>>>
>>> This would eliminate the need for some explicit type specifications
>>> for MIN2/MAX2 calls and such.
>> Sounds good.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> I think some of these numbers are semantically not so much "sizes" as
>>> "counts", and wonder if such might be better typed as uint or uintx
>>> rather than size_t.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/share/vm/gc/g1/concurrentG1RefineThread.cpp
>>>    70   _threshold = MIN2<size_t>(cg1r()->thread_threshold_step() * 
>>> (_worker_id + 1) + cg1r()->green_zone(),
>>>    73   _deactivation_threshold = MAX2<size_t>(_threshold - 
>>> cg1r()->thread_threshold_step(), cg1r()->green_zone());
>>>
>>> Are the explicit types for MIN2/MAX2 still needed here?  I think all
>>> the argument types are the same.
>> I'll fix that.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/share/vm/gc/g1/concurrentG1RefineThread.cpp
>>>   138         size_t curr_buffer_num = 
>>> (size_t)dcqs.completed_buffers_num();
>>>
>>> Identity cast.
>> Will fix.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/share/vm/gc/g1/dirtyCardQueue.cpp
>>>   214   if ((size_t)_n_completed_buffers <= stop_at) {
>>>
>>> Identity cast.
>> Will fix.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/share/vm/gc/g1/dirtyCardQueue.cpp
>>>   222     if (_completed_buffers_head == NULL)
>>>   223       _completed_buffers_tail = NULL;
>>>
>>> [preexisting]
>>>
>>> Please add braces around the "then" clause.
>> okay.
>>>
>>> Perhaps better would be to change
>>>
>>>   222     if (_completed_buffers_head == NULL)
>>>   223       _completed_buffers_tail = NULL;
>>>   224     assert(_n_completed_buffers > 0, "Invariant");
>>>   225     _n_completed_buffers--;
>>>
>>> =>
>>>
>>>    assert(_n_completed_buffers > 0, "Invariant");
>>>    _n_completed_buffers--;
>>>    if (_completed_buffers_head == NULL) {
>>>      assert(_n_completed_buffers == 0, "Invariant");
>>>      _completed_buffers_tail = NULL;
>>>    }
>> Okay.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/share/vm/gc/g1/g1RemSet.cpp
>>>   294   size_t into_cset_n_buffers = 
>>> into_cset_dcqs.completed_buffers_num();
>>>
>>> Unused variable can just be removed.
>> okay.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/share/vm/gc/g1/ptrQueue.hpp
>>>   220   size_t _process_completed_threshold;
>>>   242   size_t _max_completed_queue;
>>>
>>> These variables cannot presently be changed to an unsigned type.  A
>>> negative value is treated specially for both of them, as a flag that
>>> turns off associated behavior.  A negative _max_completed_queue means
>>> there is no maximum.  A negative _process_completed_threshold means
>>> never trigger concurrent processing of completed buffers.  I think the
>>> -1 special values (converted to size_t) end up working and providing
>>> the desired behavior by accident, which would explain why these didn't
>>> lead to any test failures.
>>>
>>> This is a bit of a mess, and I've got a fix for it in progress.
>>>
>>> Unfortunately, undoing the type changes here might make a bit of a
>>> mess elsewhere, requiring additional ugly casts.
>>>
>>> For examples of the use of -1 for these, see the constructor for
>>> SATBMarkQueue (_max_completed_queue) and the initialize call for the
>>> non-Java-thread dirty card queue set in G1CollectedHeap::initialize
>>> (both are -1).
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/share/vm/gc/g1/ptrQueue.cpp
>>>   261 size_t PtrQueueSet::completed_buffers_list_length() {
>>>   262   int n = 0;
>>>   263   BufferNode* cbn = _completed_buffers_head;
>>>   264   while (cbn != NULL) {
>>>   265     n++;
>>>   266     cbn = cbn->next();
>>>   267   }
>>>   268   return n;
>>>   269 }
>>>
>>> The type of n should also be changed to size_t.
>> okay.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>
>