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

[Aleksey Shipilev]

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: 819 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190108/e884afce/signature.asc>