RFR: 8076265: Simplify deal_with_reference

[Thomas Schatzl]

Hi Kim,

On Tue, 2015-03-31 at 15:41 -0400, Kim Barrett wrote:
> Please review this simplification and cleanup of CMTask::deal_with_reference.
> 
> I will need a sponsor for this change.
> 
> Removed _CHECK_BOTH_FINGERS_ macro, changing the code to assume it was
> true (as it always has been since it was introduced), eliminating some
> duplicated code.
> 
> Simplfied the current region tests, making use of all the associated
> variables either being set or NULL together, so no need for multiple
> NULL checks.
> 
> Added a helper function for logging and pushing objects on the mark
> stack, removing some (almost) code duplication.  This is also the
> place where some forthcoming changes for 8069367 will end up.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8076265
> 
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8076265/webrev/

  thanks for the quick turnaround.

Some comments:

 - please make sure that the parameter list style in the declaration of
new methods is similar to others. I.e. either all parameters in the same
line as the brace, or if the line gets too long, one parameter per line,
one parameter below the other.

 - what is the "mark bit processor/processing" mentioned in the comment?
Also I feel that "mark bit" is quite ambiguous, i.e. any bit on the mark
bitmap regardless of value (i.e. the bits of the mark bitmap), or does
the comment mean the mark bit in the object headers here?

At least it should be "marked bit processing" in both cases or something
like that to indicate that we are processing the bits marked (set) on
the bitmap here.

Maybe change this to "mark bitmap scanning"? Just a suggestion, there
are certainly better options.

 - would it be possible to make it explicit that we check whether we
have a current region first? Also there is opportunity to remove the
else-part of the inner if. This would make the code more understandable
imo, but feel free to ignore (and/or improve the suggested comments).

E.g.

// If the reference is within the current region we scan, handle it
// directly.
if (has_current_region() && objAddr < _region_limit) {
  // Objects past the local finger but within the current region will
  // be visited by scanning the mark bitmap for marks later. The
  // others must be processed right away.
  if (objAddr < _finger) {
    deal_with_....
  }
  return;
}
if (objAddr < _global_finger) {
  deal_with_....
}

has_current_region() could also check the _curr_region/_region_limit
invariants. Like this:

bool has_current_region() const {
  bool result = _finger != NULL;
  assert(!result || _region_limit != NULL, "...");
  assert(!result || _...);
}

 - existing typo the second long comment block:

 // Notice that the global finger might be moving forward
 // concurrently. This is not a problem. In the worst case, we
 // mark the object while it is above the global finger and, by
 // the time we read the global finger, it has moved forward
 // passed this object. In this case, the object will probably

passed -> past (I think).

 // be visited when a task is scanning the region and will also
 // be pushed on the stack. So, some duplicate work, but no
 // correctness problems.

 - I do not really like an extra parameter in
deal_with_reference_below_finger() just for logging, but I do not see a
much better option except that right now.

 - in deal_with_reference_below_finger(), the indentation of the
arguments to gclog_or_tty->print_cr() is non-standard.

Thanks,
  Thomas