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

[Tom Benson]

Hi Joe,

On 2/19/2016 11:48 AM, Joseph Provino wrote:
>
>
> On 2/19/2016 11:25 AM, Tom Benson wrote:
>> 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.
> I'll fix that.

Well, not to nit-pick 8^),   but your new webrev reverted this to intx, 
but I think it would make a nice uintx , as I said.   As long as you're 
there.  Or leave it, your call.

>>
>> 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^)
> accident!  I'll fix it.  Should it be SIZE_MAX?
>

I think so, and I see that's what you did.

Tom

> joe
>
>>
>> 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.
>>>>>
>>>>> ------------------------------------------------------------------------------ 
>>>>>
>>>>>
>>>>
>>>
>>
>