RFR (M): 8071278: Fix the closure mess in G1RemSet::refine_card()

[Thomas Schatzl]

Hi Stefan,

On Fri, 2017-02-24 at 14:07 +0100, Stefan Johansson wrote:
> Thanks for cleaning this up Thomas,
> 
> On 2017-02-23 12:23, Thomas Schatzl wrote:
> > 
> > Hi all,
> > 
> >    one line:
> > 
> > "188 lines changed: 23 ins; 143 del; 22 mod; 3097 unchg"
> > 
> > This is a rip-out and manual collapsing of needlessly complex code
> > in the update rs algorithm.
> > 
> > There are more improvements obvious now in that area (JDK-8071280,
> > JDK-8162928, JDK-8175554 at least), so I kept this one to the
> > minimum.
> > 
> > The change makes the code a bit faster too due to avoiding repeated
> > null-checks, region-crossing checks etc. etc.
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8071278
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8071278/webrev/
> Change looks good in general, just a few small things.
> ---
> src/share/vm/gc/g1/g1_specialized_oop_closures.hpp:
>   45       f(G1UpdateRSOrPushRefOopClosure,_nv)              \
> 
> Alignment of '\'.

Fixed.

> ----
> src/share/vm/gc/g1/g1OopClosures.hpp:
> 192   bool apply_to_weak_ref_discovered_field() { return true; }
> 
> Did you do any analysis whether or not this is needed? Or did you
> just re-add it since FilterOutOfRegionClosure had it? I never figured
> out a clear answer to that when I was looking at this a while back. I
> don't see any need to change this right now, just curious if you know
> why it is needed.

I did only do cursory analysis about that, but it seems that some code
(G1CollectedHeap::preserve_cm_referents(), pending list management)
relies on it, so it needs to be preserved, and so simply added it when
removing the FilterOutOfRegionClosure you described.

I will think about it again in more detail soonish.

Thanks,
  Thomas