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 16:05:02 UTC 2016


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