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 17:04:57 UTC 2016
New webrev is here: http://cr.openjdk.java.net/~jprovino/8139651/webrev.02
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.
>
> 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.
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>
>>
>
More information about the hotspot-gc-dev
mailing list