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