RFR: 8166607: G1 needs klass_or_null_acquire

[Thomas Schatzl]

Hi Kim,

On Mon, 2016-11-07 at 14:38 -0500, Kim Barrett wrote:
> > 
> > On Nov 7, 2016, at 5:53 AM, Thomas Schatzl <thomas.schatzl at oracle.c
> > om> wrote:
> > On Tue, 2016-10-25 at 19:11 -0400, Kim Barrett wrote:
> > > 
> > > > 
> > > > 
> > > > On Oct 21, 2016, at 9:54 PM, Kim Barrett <kim.barrett at oracle.co
> > > > m>
> > > > wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > On Oct 21, 2016, at 8:46 PM, Kim Barrett <kim.barrett at oracle.
> > > > > com>
> > > > > wrote:
> > > > > In the humongous case, if it bails because klass_or_null ==
> > > > > NULL,
> > > > > we must re-enqueue
> > > > > the card …
> > > This update (webrev.02) reverts part of the previous change.
> > > 
> > > In the original RFR I said:
> > > 
> > >   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.
> > > 
> > > Ignoring such a stale card is incorrect at the point where it was
> > > being done.  At that point we've already cleaned the card, so we
> > > must
> > > either process the designated object(s) or, if we can't do the
> > > processing because of in-progress allocation (klass_or_null
> > > returned
> > > NULL), then re-queue the card for later reconsideration.
> > > 
> > > So the change to refine_card to eliminate that behavior, and the
> > > associated changes to oops_on_card_seq_iterate_careful, were a
> > > mistake, and are being reverted by this new version.  As a
> > > result,
> > > refine_card is no longer changed at all.
> > Thanks for catching this.
> > 
> > Maybe it would be cleaner to call a method in the barrier set
> > instead of inlining the dirtying + enqueuing in lines 685 to 691?
> > Maybe as an additional RFE.
> We could use _ct_bs->invalidate(dirtyRegion).  That's rather
> overgeneralized and inefficient for this situation, but this
> situation should occur *very* rarely; it requires a stale card get
> processed just as a humongous object is in the midst of being
> allocated in the same region.

I kind of think for these reasons we should use _ct_bs->invalidate() as
it seems clearer to me. There is the mentioned drawback of having no
other more efficient way, so I will let you decide about this.

> > > Additionally, in the original RFR I also said:
> > > 
> > >   Note that [...] At present the only source of stale cards in
> > > the concurrent case seems to be HCC eviction.  [...]  Doing HCC
> > > cleanup when freeing regions might remove the need for
> > > klass_or_null checking in the humongous case for concurrent
> > > refinement, so might be worth looking into later.
> > > 
> > > That was also incorrect; there are other sources of stale cards.
> > Can you elaborate on that?
> Here's a scenario that I've observed while running a jtreg test (I
> think it was hotspot/test/gc/TestHumongousReferenceObject).
> 
> We have humongous object H, referring to young object Y.  This
> induces a remembered set entry for card C in region R (allocated for
> H).
> 
> H becomes unreachable.
> Start concurrent collection cycle.
> Pause Initial Mark scan_rs pushes &H->Y onto mark stack.
> Pause Initial Mark evac processes &H->Y, copying Y, updating &H->Y,
>   and adding C to g1h_dcqs in update_rs.
> Pause Initial Mark redirty_logged_cards dirties g1h_dcqs entries,
> including C.
> Pause Initial Mark merges g1h_dcqs into java_dcqs, adding dirty C to
> java_dcqs.
> Concurrent Mark determines H is dead.
> Pause Cleanup frees regions for H, including R.
> Concurrent Refinement finally comes across stale C in now (possibly)
> free R.
> 
> A similar situation can arise if instead of H we have old O in region
> R and all objects in R are unreachable before starting concurrent
> collection, so that Pause Cleanup frees R.

Okay, thanks, understood.

Thomas