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

Kim Barrett kim.barrett at oracle.com
Fri Feb 12 23:49:40 UTC 2016


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

------------------------------------------------------------------------------

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.

------------------------------------------------------------------------------

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.

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/concurrentG1RefineThread.cpp 
 138         size_t curr_buffer_num = (size_t)dcqs.completed_buffers_num();

Identity cast.

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/dirtyCardQueue.cpp 
 214   if ((size_t)_n_completed_buffers <= stop_at) {

Identity cast.

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

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;
  }

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

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

------------------------------------------------------------------------------



More information about the hotspot-gc-dev mailing list