RFR (S): 8066102: Clean up HeapRegionRemSet files

Thomas Schatzl thomas.schatzl at oracle.com
Mon Dec 1 12:07:53 UTC 2014


Hi,

On Fri, 2014-11-28 at 16:18 -0500, Kim Barrett wrote:
> [resending from proper from address, so it will reach mailing list]
> 
> On Nov 28, 2014, at 3:46 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> > 
> > Hi all,
> > 
> > can I have reviews for this small cleanup? I went over heap region
> > remembered set files recently and found dead code, useless code,
> > tightened the public interface and tried to document the methods a
> > little. I also moved FromCardCache accessor methods from
> > OtherRegionTable to HeapRegionRemSet to do away with that needless
> > indirection. Imo it fits better that way too.
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8066102
> > 
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8066102/webrev/
> 
> Looks good.
> 
> This change surprised me though:
> 
> src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp
> 978   _bosa(hrrs->bosa()),
> =>
> 943   _bosa(hrrs->_bosa),
> 
> Violates the Hotspot coding guidelines item about member access from
> outside the class be through accessor functions.  OTOH, the class
> being accessed has befriended the class doing this access, and the
> (removed by this changeset) bosa() accessor was private.

That's the reason why I removed that. I will keep it as is if nobody
objects.

> 
> Regarding Mikael's comment:
> > In heapRegionRemSet.hpp:
> > +  // One bits in the bitmaps indicate that the given region or card is live.
> > typo: "One bit in one of the bitmaps indicate that the.."
> 
> I think the existing comment is correct, but might be clearer if it
> said
> 
> "Set bits in the bitmaps ..."
> 

Fixed.

http://cr.openjdk.java.net/~tschatzl/8066102/webrev.1 (this is a full
webrev again, really only fixed that comment).

Thanks,
  Thomas





More information about the hotspot-gc-dev mailing list