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

[Roman Kennke]

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
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190108/4e858cb6/signature.asc>