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
Thu Feb 18 18:44:01 UTC 2016
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