RFR: Handle checkcast arraycopy correctly in barriers. Make arraycopy+barriers a single loop.

Roman Kennke rkennke at redhat.com
Tue Feb 27 13:18:26 UTC 2018


Hi Aleksey,

>> Which is implemented here:
>> http://cr.openjdk.java.net/~rkennke/arraycopy-loop/webrev.00/
>
> *) The test should capture bugs in this code. It should be randomized, *and* it should validate what
> have been written with the copy itself, that would save us time debugging the cryptic failures in
> larger scenarios. It should probably be separate from checkcast test.

Ok, I added another test that does some randomized conjoint
arraycopying and verified that it exposes the bugger that I had during
the making.

> *) It is a good style to split the line by "static"/"dynamic" parameters. In other words, I should
> *see* the structure of the signature, e.g.:
>
>    template <typename T>
>    bool arraycopy_loop_1(T* src, T* dst, size_t length, Klass* bound,
>                          bool checkcast, bool satb, bool matrix,
>                          ShenandoahBarrierSet::ArrayCopyStoreValMode storeval_mode);
>
>
> *) No need to put comments like "/* checkcast */" in the expansion, it is evident what template
> parameter you are juggling, and it only adds to clutter, do e.g.:
>
>   99   if (checkcast) {
>  100     return arraycopy_loop_2<T, true>(src, dst, length, bound, satb, matrix, storeval_mode);
>  101   } else {
>  102     return arraycopy_loop_2<T, false>(src, dst, length, bound, satb, matrix, storeval_mode);
>  103   }
>
> *) Aren't switches indented wrong? "case" should be indented inside the switch block. Select the
> switch block, and do Ctrl+Alt+L.
>
> *) Isn't storeval_str switch redundant?
>
> *) The asserts for every single "other" state should probably just assert the exact state we want to
> be in.
>
> Stylistic changes:
>  http://cr.openjdk.java.net/~shade/shenandoah/arraycopy-shade-fixes.patch

Thanks for your changes. One of the asserts was too strict: it is ok
to not do any storeval-barrier if conc-mark is in progress and no refs
need updating. I fixed that.

Differential (against your changes):
http://cr.openjdk.java.net/~rkennke/arraycopy-loop/webrev.01.diff/
Full (incl. your changes plus my additional test+fixes):
http://cr.openjdk.java.net/~rkennke/arraycopy-loop/webrev.01/

Good now?

Roman


More information about the shenandoah-dev mailing list