RFR: 8165808: Add release barriers when allocating objects with concurrent collection

[Kim Barrett]

> On Sep 12, 2016, at 5:53 PM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
> 
> 
> 
> On 09/12/2016 11:08 AM, Kim Barrett wrote:
>>> On Sep 11, 2016, at 9:38 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> Hi Kim,
>>> 
>>> On 11/09/2016 8:59 AM, Kim Barrett wrote:
>>>> Please review this change to add release barriers when allocating
>>>> objects that may be visible to a concurrent collector.
>>>> 
>>>> This change only addresses the allocation side, where the
>>>> initialization occurs; other subtasks of JDK-8160369 will address the
>>>> (mostly GC-specific) readers that need to accomodate in-progress
>>>> allocations.
>>>> 
>>>> As part of this change, eliminated post_allocation_install_obj_klass,
>>>> which consisted of a call to [release_]set_klass + assertions that are
>>>> redundant with those in that called function.
>>>> 
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8165808
>>>> 
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~kbarrett/8165808/webrev.00/
>>> src/share/vm/gc/shared/collectedHeap.inline.hpp
>>> 
>>> Based on experiences with RMO architectures (ref JDK-8033920 - sorry it is a non-open bug report) I would expect the use of release_set_klass to be unconditional, not dependent on the nature of the GC. Publication of the object reference also has to be safe for other mutator threads. I'm not convinced by the brief argument in JDK-8033920 that thread-state transitions will always ensure visibility and ordering.
>> There were two different issues discussed in (non-open) JDK-8033920.
>> 
>> (1) Ensure setting the klass happens after some other parts of the
>> object initialization, to ensure (some) concurrent GCs can parse the
>> heap.
>> 
>> (2) Ensure the klass is visible to other (mutator) threads if the
>> object is accessible to those threads, e.g. if the object escapes from
>> the allocating thread.
>> 
>> JDK-8160369 is only about (1), and this change (for JDK-8165808) is
>> part of fixing (1).  I'm not trying to address (2) here, even assuming
>> there are problems in that area.  (I don't know that there *are*
>> problems; I haven't explored that question carefully. I did find
>> storestore barriers in the appropriate places in the bytecode
>> interpreter, but I don't know how the reader side works, nor have I
>> examined relevant parts of the compilers.)
> 
> In the above context (release store part of 8165808), changes look
> good.
> 
> Jon

Thanks.

> 
>> 
>> For (1), only concurrent GCs are affected.  A purely STW GC can rely
>> on safepointing to ensure the ordering.  (Safepointing also immunizes
>> STW GCs from (2).)
>> 
>> The parsability constraint for CMS and G1 is a consequence of
>> concurrent processing of dirty card information (CMS precleaning and
>> G1 concurrent refinement), which needs to be able to find object
>> boundaries in the regions covered by marked cards.  Hence, for both
>> CMS and G1, only allocations directly in the old generation need
>> special care.  And for G1, only humongous objects are directly
>> allocated in the old-gen.
>> 
>> [I don't know enough about Shenandoah to know whether it has any
>> similar requirements.]
>> 
>> This is the rationale for the conditionalization on INCLUDE_ALL_GCS.
>> 
>> It is also the rationale for only doing the release_set_klass in the
>> collectedHeap.inline.hpp code paths, and not the TLAB / Eden
>> allocation paths.
>> 
>> It would be possible to add runtime conditionalization, but that could
>> quickly add more cost than just executing the barrier unconditionally.
>> Especially on TSO platforms :-) And these are slow-path allocations,
>> used only when TLAB / Eden allocation is disallowed or fails, so an
>> unnecessary release barrier shouldn't have much overall impact even on
>> non-TSO platforms.  Hence keeping it simple.