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

Joseph Provino joseph.provino at oracle.com
Fri Feb 19 18:39:16 UTC 2016



On 2/19/2016 1:16 PM, Tom Benson wrote:
> 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.
Yeah, I put it back to the way it was.  But if you think it should be 
uintx, I'll change it...

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



More information about the hotspot-gc-dev mailing list