RFR (XXS): JDK-8085983 G1CollectedHeap::collection_set_iterate_from() has unused code and can be simplified

[Thomas Schatzl]

Hi Joe,

On Wed, 2015-08-05 at 12:28 -0400, Joseph Provino wrote:
> Latest webrev is here:
> http://cr.openjdk.java.net/~jprovino/8085983/webrev.01

I assume this is the latest version of the change; see below for
comments.
> 
> joe
> 
> On 6/18/2015 10:52 AM, Joseph Provino wrote:
> 
> > 
> > On 6/10/2015 12:46 PM, Stefan Karlsson wrote:
> > 
> > > Hi Joe,
> > > 
> > > On 2015-06-08 21:16, Joseph Provino wrote:
> > > 
> > > > One file. 
> > > > 
> > > > CR:  https://bugs.openjdk.java.net/browse/JDK-8085983 
> > > > 
> > > > webrev:  http://cr.openjdk.java.net/~jprovino/8085983/webrev.00 
> > > 
> > > Some comments:
> > > 
> > > +     guarantee(!cl->doHeapRegion(cur), err_msg("doHeapRegion() returned true"));
> > > 
> > > - You don't need err_msg if you don't have any format specifiers
> > > in the error message.
> > > 
> > I'll fix that.
> > 
> > > - I would prefer if you didn't put the cl->doHeapReagion() calls
> > > inside guarantees. It's too easy to incorrectly assume that the
> > > code in the guarantee is only verification code, IMHO.

That should also be true for the other guarantees imho, not only the one
in G1CollectedHeap::collection_set_iterate().
> > > 
> > That's a good point.  Something like this:
> > 
> >     bool ret = cl->doHeapRegion(r);
> >     guarantee(ret == false, "doHeapRegion() returned true");
> > 
> > Or do you prefer !ret?

We do not compare to true/false directly, i.e. !ret is the choice here.
> > 
> > > - Shouldn't this be an assert instead of a guarantee?
> > I don't know the rules there.  Seems like assert would be better
> > so it doesn't slow down the product build.

Either is fine with me. Since this is work distribution on a per-region
basis, so an additional check very likely is unnoticeable.

Thanks,
  Thomas