RFR: 8172157: Tighten up contract between concurrent GCs and runtime regarding object allocation and header initialization

[Christine Flood]

Shenandoah is not currently generational, so the comment about tlab allocations being only
in the young generation is not applicable to us.

We scan the object graph concurrently.  Assuming that a reference to an object isn't available
until after the object is initialized, and that all objects are correctly formatted at safepoints
we should be fine.

The issue would only come up when walking heap regions.  We only do that when we are in an evacuation
phase and the region is in the collection set.  We don't allow allocations in regions targeted for
evacuation.

I'm not 100% sure I am grasping all of the subtleties. 


Christine




----- Original Message -----
> From: "Kim Barrett" <kim.barrett at oracle.com>
> To: "Derek White" <Derek.White at cavium.com>
> Cc: hotspot-dev at openjdk.java.net, "Christine Flood" <cflood at redhat.com>
> Sent: Wednesday, January 4, 2017 8:22:45 PM
> Subject: Re: RFR: 8172157: Tighten up contract between concurrent GCs and	runtime regarding object allocation and
> header initialization
> 
> > On Jan 4, 2017, at 6:02 PM, White, Derek <Derek.White at cavium.com> wrote:
> > 
> > [resending to hotspot-dev]
> > 
> > There are three issues that came up in the review comments for 8171449 that
> > I tried to handle:
> > 1) Inline allocation by jitted code and the interpreter only allocate out
> > of the young gen (either TLABS or eden), and the concurrent GC threads
> > will never scan object in the young gen, so inline-alloctors don't need to
> > ensure memory ordering for the concurrent GC's use. Added comments and
> > assertions.
> >  
> > 2) Non-inline allocation in the old gen by a concurrent GC should first set
> > an object’s klass field to NULL before setting it to a correct value.
> > Added comments and assertions.
> > 3) There should be no race condition between the steps in 2 that allows a
> > concurrent GC thread to see an incorrect value. This is harder to prove,
> > and is not handled in this fix.
> >  
> > I'm trying to pull some of the constraints out from the depths of the
> > collectors to something closer to the GC/runtime interface, so the non-GC
> > code (writers) can reason about it.
> >  
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8172157 “Tighten up contract
> > between concurrent GCs and runtime regarding object allocation and header
> > initialization”
> >  
> > Webrev: http://cr.openjdk.java.net/~drwhite/8172157/webrev.02/
> >  
> > Thanks,
> >  
> > 	• Derek
> 
> Note that I think this sort of thing is an enhancement, rather than a
> bug as the JIRA issue is presently categorized.  Nothing is broken;
> it's just harder to understand and modify the code than we might
> prefer.  As such, I think work along this line ought to be deferred
> out of JDK 9.
> 
> ------------------------------------------------------------------------------
> src/share/vm/gc/shared/collectedHeap.hpp
>  132   // Concurrent collectors must never allocate a TLAB from the old
>  generation,
>  133   // because inline-allocation in TLABS by jitted code does not enforce
>  134   // memory ordering consistency for store-length/store-klass, so
>  concurrent GC
>  135   // could see inconsistent values when scanning the old gen.
> 
> This comment is wrong for a single generation concurent collector.
> Such a collector might still support TLAB allocation for performance.
> It would just have arrange to not look in the TLABs when that would be
> a problem.  Consider a SATB-based collector, where allocation is
> black; do something like make TLAB portions from before start of mark
> parsable, and don't scan objects in TLAB portions (possibly entire
> TLABs) allocated from after start of marking.  I think Shenandoah
> might be an example of this, and similarly for a collector I worked on
> for a former employer.
> 
> ------------------------------------------------------------------------------
> src/share/vm/gc/shared/genCollectedHeap.cpp
> 1019   assert(is_in_young((oop)result), "TLAB must be allocated in young
> gen");
> 
> I think this assertion is wrong, for the same reason I think the
> comment for allocate_new_tlab is wrong.
> 
> ------------------------------------------------------------------------------
> src/share/vm/gc/shared/genCollectedHeap.cpp
>  492       assert(is_in_young((oop)result) ||
>  493              ((oop)result)->klass_or_null() == NULL,
>  494              "mem_allocate must clear (what will be) the klass field for
>  non-young allocations");
> 
> I think a more accurate assertion regimen would be to verify
> non-humongous allocations were made from the young gen, and humongous
> allocations have the needed null klass location.
> 
> ------------------------------------------------------------------------------
> 
> I'm generally suspicious of the proposed comments and assertions.  I
> think the actual contracts are not as simple :) as suggested by these
> changes.
> 
> For example, for G1, some of the relevant requirements around setup
> and ordering on the allocation side appear to be to support concurrent
> refinement.  If using the suggested G1 throughput mode (simpler
> post-barrier, no concurrent refinement), some of those requirements
> might be relaxed, though we might not conditionalize the code.
> 
> And for G1 generally, some of the ordering being done on the slow path
> (such as ensuring the length of arrays and classes are set before
> setting the klass field) is only needed when the object is humongous,
> but again here the code isn't conditionalized.
> 
> So I think these kind of changes are premature.  I think what is first
> needed is some discussion of what the contracts really are, e.g. reify
> the design that is presently implicit in the code.  Ideally, this
> should identify parts that are specific to certain GCs or GCs having
> certain properties.  Once we have that, then we can start talking
> about how to feed that information back into the code, in the form of
> assertions, conditionalizations, &etc.
> 
> An additional benefit of this would be discovery of code bits that
> might be conditionalized as part of the GC abstration layer that has
> been discussed recently.  It might also help identify "shared" code
> that is really CMS-specific, in support of CMS deprecation.
> 
>