RFR: 8166607: G1 needs klass_or_null_acquire

[Erik Helin]

On 2016-10-13, Kim Barrett wrote:
> > On Oct 13, 2016, at 4:39 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> >> On Oct 13, 2016, at 6:11 AM, Erik Helin <erik.helin at oracle.com> wrote:
> >> 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.
> > 
> > I'm pretty sure I don't want to analyze the changes I'm thinking about
> > making for JDK-8166711 against the old (pre-fix for JDK-8166607) code.
> > So re-ordering the changes doesn't really work for me.
> > 
> > That's why I suggested I could, in the webrev under review, add an
> > OrderAccess::loadload() somewhere between the top() access and the
> > is_young() check to ensure the ordering there, as a temporary
> > workaround for JDK-8166711.
> 
> Note that we’re using the wrong bug number for that other bug.  It’s
> JDK-8166811.
> 
> That loadload addition wouldn’t actually work; I forgot that the needed store-side
> ordering went missing at some point.
> 
> >  Or you could pretend it's there when
> > reviewing (which is what I did when writing it).
> 
> So what I’d like is that you just pretend for now that the comment about the
> ordering in oops_on_card_xxx is actually correct.
> 
> The change I’m working on for JDK-8166811 eliminates that ordering problem
> completely, but depends heavily on the changes here.

I've discussed this with Thomas, what do you think about pushing the
fix for JDK-8166811 at the same time as this one? This patch looks good
AFAICS, but I'm worried that your code will be exposed slightly
different to existing bug than the current code. Given that
crashes/issues resulting from these kinds of bugs often are nightmarish
to track down, I'm a bit hesitant to push these patches one by one.

What do you think?

Thanks,
Erik