RFR (S) 8215724: Epsilon: ArrayStoreExceptionTest.java fails; missing arraycopy check

[Erik Österlund]

Hi Roman and Aleksey,

I suppose I can see both points of view. I have reconsidered, and am 
okay with adding this to BarrierSet instead, given Aleksey's rationale.

Looks good.

Thanks,
/Erik

On 2019-01-08 17:24, Roman Kennke wrote:
> FWIW, I have to agree with Aleksey here. Either make superclass abstract
> or provide a usable implementation for GCs that don't need anything
> special (e.g. Epsilon).
> 
> Roman
> 
>> Hi Erik,
>>
>> On 1/8/19 12:50 PM, Erik Österlund wrote:
>>> The decision to have GCs be explicit about their choice of check-cast arraycopy was intentional.
>>
>> Mmm. That intent is entirely not clear. If the intent was to force GC implementation to come up with
>> its own arraycopy handling, then BarrierSet::oop_arraycopy_in_heap had to be Unimplemented(). That
>> would make the choice "explicit". Instead, the default implementation silently/implicitly calls to
>> Raw::oop_arraycopy -- that is a bug, and that is the reason Epsilon broke the Java semantics.
>>
>>> So I think the implementation should move into an epsilon barrierset inline hpp file.
>>
>> I remember when I was doing the EpsilonBarrierSet an year ago, you told me to delegate stuff to
>> superclass, because no-op GC can use whatever the default implementation is doing. At the time, I
>> thought it is a mere development convenience, but today I think this is actually a good GC API
>> design: the default implementation does the minimal+correct thing, and does not break Java
>> semantics. So if GC does not need anything special, it can use all the defaults. Which means, among
>> other things, that one of the sanity tests for the GC API is having empty EpsilonBarrierSet.
>>
>>
>>> The reason is that every GC seems to want their own optimized version of arraycopy with checkcast,
>>> handling the fact that a partial range of writes was committed. For the write barrier GCs (ModRef),
>>> they have their own partial card range dirtying mechanism, for ZGC, there is a load barrier per
>>> element, Shenandoah has its own interesting looking template thing. So the variation of arraycopy
>>> that performs checks covariance but does not apply any GC barriers (i.e. raw loads and stores), will
>>> only be used by Epsilon and would most likely be an error if it was used for any other GC.
>>
>> True. If GC needs something special w.r.t. barriers and some such, it has to override BS methods. If
>> GC does not need special handling, the default implementation has to do the right thing. The
>> super-class implementation also serves as the basis what should a concrete GC implementation
>> minimally do to maintain Java semantics. It is actually the same way with other methods in
>> BarrierSet, as far as I see it (for example, xchg does atomics/locking, stores actually store, loads
>> actually load) and I don't see the reason why arraycopy should be special and not do what is
>> expected by Java rules (copying with checking casts).
>>
>>> Do you agree?
>>
>> I have to disagree. I still think default BarrierSet should maintain Java semantics by providing the
>> minimal, but still correct code. We can implement the fix in EpsilonBarrierSet, but it just works
>> around the awkward special case in GC interface, diluting the GC interface itself, and I see no
>> compelling reason to accept that.
>>
>> Cheers,
>> -Aleksey
>>
>