RFR: 8189871: Refactor GC barriers to use declarative semantics

[Erik Österlund]

Hi Kim,

On 2017-11-23 21:23, Kim Barrett wrote:
>> On Nov 21, 2017, at 2:50 PM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> Hi Kim,
>>
>>
>> On 2017-11-21 19:48, Kim Barrett wrote:
>>>> On Nov 21, 2017, at 11:20 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>>>
>>>> Hi Kim,
>>>>
>>>> I am merely more precisely annotating that unsafe accesses on oops have explicitly unknown oop ref strength. However, the GC backends do not do more about it than they did before. I.e. only G1 loads check the actual reference strength and act accordingly (as before), and other exotic accesses are treated the same in the backends (as before) since their pre-write barriers are enough anyway. The behaviour is the same before and after.
>>> For the operations in question, there were no checks for reference
>>> strength before, not even for G1. So the behavior is not the same
>>> before and after the Access changes.
>>>
>>> Unsafe_CompareAndExchangeObject on a referent was effectively UB. I
>>> don't think adding the special handling for referents makes it any
>>> less UB; it's still an abuse of Unsafe to break invariants. In which
>>> case, any potential additional work that might be done to handle that
>>> specific case is not just pointless, but actively harmful.
>> For the operations in question, there were no checks for reference strength before or after. The unsafe.cpp file annotates that this is an unknown oop ref, the backends don't check for it or care. Seems very much the same to me. Am I missing something?
> Apparently not.  I was sure I saw a call to
> resolve_possibly_unknown_oop_ref_strength in the relevant code path.
> That seems to have been a mirage.  Sorry for the noise.

No problem. I am glad we agree on this point.

> However, while re-exploring that code path, I did notice something
> that bothered me.
>
> ModRefBarrierSet::AccessBarrier<...>::oop_atomic_cmpxchg_in_heap
>    bs->template write_ref_field_pre<decorators>(addr);
>    oop result = Raw::oop_atomic_cmpxchg(...);
>    if (result == compare_value) {
>      bs->template write_ref_field_post<decorators>(...);
>    }
>    return result;
>
> does an extra load for G1, to apply the pre-barrier.  The old code
> applied the pre-barrier to the result obtained from the raw cmpxchg.

In my Access API, I want the notion of pre and post write barriers 
introduced on ModRef level to be respected as just that - pre and post 
write barriers. Previous code assumed that pre-write barrier really has 
nothing to do with whether it comes before or after the write - instead 
it was assumed that a pre-write barrier is equivalent to the SATB 
barrier, to the extent that in some places the pre-write barrier is used 
as an implementation for Reference.get for G1 (a weak load!), because it 
happens to be a SATB barrier and has nothing to do with happening before 
a write.

So to on the ModRef level assume that pre-write barriers can be 
performed *after* the write, because we know it is an SATB barrier, is 
an abstraction crime. That is why I apply it before the write instead 
like we do for stores - because it is a pre-write barrier (and if it 
wasn't expensive in the context of a store, I see no point in optimizing 
it in the context of an atomic CAS). If a specific GC wants to apply it 
after as a micro optimization, then they should override the ModRef 
implementation for their CAS to do that.

To apply the pre-write barrier *after* the write for CAS specifically, 
is an optimization to get rid of a relaxed load from a conservatively 
fenced atomic operation, in a cold runtime path. That seemed like not a 
good idea to micro optimize and violate abstractions for to me. If we 
really want to optimize that code path, I see more point in applying the 
xor shift filter to not perform the expensive StoreLoad fence and card 
fiddling for writes within the same old region in the runtime path of 
the post-write barrier, as we do in compiled code.

I hope you agree that performing the pre-write barrier as a post-write 
barrier for the rather chilly runtime CAS is a bad idea and not an 
optimization that is worth it.

Thanks,
/Erik