RFR: 8166607: G1 needs klass_or_null_acquire

[Erik Helin]

On 2016-10-10, Kim Barrett wrote:
> > On Oct 10, 2016, at 9:54 AM, Erik Helin <erik.helin at oracle.com> wrote:
> > 
> > On 2016-09-29, Kim Barrett wrote:
> >> Please review this change to G1 to use klass_or_null_acquire where
> >> needed.
> >> 
> >> […]
> >> 
> >> Changed oops_on_card_seq_iterate_careful to use different code paths
> >> when the region is humongous or not.
> >> 
> >> The humongous case is simplified because there can only be the one
> >> (potential) object in the region.  Use klass_or_null_acquire to
> >> determine whether the object has been "published" or allocation is
> >> still in-progress.  The acquire barrier ensures processing the
> >> humongous object is safe.  When the klass is NULL the card marking
> >> must be stale and can be ignored.
> >> 
> >> In the non-humongous case, we know we're dealing with an old-gen
> >> region.  Because of that, we know we don't need to deal with
> >> in-progress allocations here.  That allows many simplifications.
> >> 
> >> As a result of the changes in oops_on_card_seq_iterate_careful, we now
> >> almost never fail to process the card.  The only place where that can
> >> occur is a stale card in a humongous region with an in-progress
> >> allocation, where we can just ignore it.  So the only caller,
> >> refine_card, no longer needs to examine the result of the call and
> >> enqueue the card for later reconsideration.
> > 
> > I'm not sure this assumption is correct. Due to JDK-8166711 [0], can't
> > we have the following scenario:
> > 
> > […]
> > What do you think? This is very tricky code to analyze, but I think this
> > patch won't work in the above scenario.
> > 
> > When JDK-8166711 [0] is solved however, then I do think that this patch
> > will work. The important part for getting this patch to work is to ensure
> > that the two first checks, mr.is_empty() and is_young(), aren't
> > reordered.
> 
> I think the existing code can also fail in such a JDK-8166711 scenario.

Yep! :)

On 2016-10-10, Kim Barrett wrote:
> I don’t think my proposed changes make that situation any worse, and will
> enable changes I think can be made to fix 8166711 that will both simplify things
> further and might not require any additional barriers.  Note that 8166711 is the
> next thing I plan to work on.  I could add a loadload barrier as a temporary
> stopgap; that didn’t seem necessary since the failures are pretty low probability
> (not sure we’ve ever seen one) and I’m going to work on what I think will be
> a better fix for that problem next.

I agree that your change doesn't make it worse and I know that you will
work on JDK-8166711. The problem is that I get headache enough from
reasoning about these kind of issues, so I'm having a hard time
reviewing this patch and trying to figure if it is correct as it is. If
I also have to take into account that some known issues will be fixed,
then it becomes *really* hard for me to verify this patch :(

If you plan on working on JDK-8166711, is there any chance you can send
out the patch for that issue first, and then this patch can be
rebased on top of that work? I will of course review the patch for
JDK-8166711 as well.

Thanks,
Erik