8245121: (bf) XBuffer.put(Xbuffer src) can give unexpected result when storage overlaps

[Paul Sandoz]

This is looking good,

Strictly speaking the fences are only required for the direct buffer, but I think I prefer them being placed where the unsafe usages are for tighter scope.

I would be inclined to drop the Bits.JNI_COPY_TO_ARRAY_THRESHOLD, since it never applies to direct buffers. I bet the swapping. Maybe can be determined with follow evaluation after this issue.

Test generally looks good:

  47     static final Random RND = new MyRandom(SEED); // XXX RandomFactory
  48 
  49     static class MyRandom extends Random {
  50         MyRandom(long seed) {
  51             super(seed);
  52         }
The additional methods are never used. No need to extend? (If there was a need the methods could use next accepting bits values).

I still hold out hope you can convert it to a testng test, with data providers and asserts, this is all good common practice for writing tests. The nested loops in test() effectively capture the test combinations input for two test cases. Let me know if you need some advice/help on this.

Paul.

> On May 19, 2020, at 2:06 PM, Brian Burkhalter <brian.burkhalter at oracle.com> wrote:
> 
> Here [1] is an updated version of the proposed patch. This version adds fences to the X-Buffer version of put($TypeBuffer) and modifies the direct and heap implementations to call super.put().
> 
> Specification verbiage is also added:
>      * If this buffer and
>      * the source buffer share the same backing memory, then the result will
>      * be as if the source elements were first copied to an intermediate
>      * location before being written into this buffer.
> If this work goes forward and this doc change is deemed necessary / worthwhile then a CSR will be filed.
> 
> Some benchmarks were run to measure the performance of putting the contents of direct, heap, direct view, swapped direct view, heap view, and swapped heap view IntBuffers into direct and heap IntBuffers. The results are at [2]. On the average the proposed version appears to perform better, sometimes substantially, than the current version. A notable exception is for writing views of a byte buffer into a heap IntBuffer for a very small capacity. One would expect that this is due to falling through to the loopy implementation for small copy lengths, but testing did not bear this out. Some more investigation might be needed here.
> 
> Thanks,
> 
> Brian
> 
> [1] http://cr.openjdk.java.net/~bpb/8245121/webrev.01 <http://cr.openjdk.java.net/~bpb/8245121/webrev.01>
> [2] http://cr.openjdk.java.net/~bpb/8245121/XBufferBench.csv <http://cr.openjdk.java.net/~bpb/8245121/XBufferBench.csv>
>> On May 15, 2020, at 12:42 PM, Paul Sandoz <paul.sandoz at oracle.com <mailto:paul.sandoz at oracle.com>> wrote:
>> 
>> As you say it might be worth performing some benchmark, and be more informed whether the fallback to the loop is necessary. 
> 
> 
> 
>> On May 18, 2020, at 12:57 AM, Alan Bateman <Alan.Bateman at oracle.com <mailto:Alan.Bateman at oracle.com>> wrote:
>> 
>> The approach looks reasonable but I suspect this will need a reachability fence for at least src (maybe this too, I need to page some of the details).
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200526/117de836/attachment.htm>