RFR [15] 8247696: Incorrect tail computation for large segments in AbstractMemorySegmentImpl::mismatch

[Chris Hegarty]

Hi Paul,

> On 17 Jun 2020, at 21:56, Paul Sandoz <paul.sandoz at oracle.com> wrote:
> 
> Hi Chris,
> 
> AbstractMemorySegmentImpl
> —
> 
> In vectorizedMismatchLarge:
> 
> 163             if (remaining > 7)
> 164                 throw new InternalError("remaining greater than 7: " + remaining);
> 165             i = length - remaining;
> 166         }
> 
> Should this check be an assert?

It is logically an assertion of vectorizedMismatchLarge, but assertions are not used elsewhere in the foreign memory code. Arguably the check is not needed at all, we could choose to just drop it.

> —
> 
> This fix prompted me to think more deeply about the implementation of vectorizedMismatchLarge and its use of vectorizedMismatch. Sorry, I should have thought about this more throughly earlier on.
> 
> We need to refine the approach, not for this patch, but something to follow up after.

Ok, I’ll an issue to track these.

> I think there are two issues.
> 
> 1) The intrinsic method vectorizedMismatch could potentially bomb out at any point and return the bitwise compliment of the remaining number of elements to check.
> 
> Obviously, there is no point doing that arbitrarily but a stub implementation for, say, x86 AVX-512 might decide bomb out for  < 64 remaining elements, rather than apply vector operations on smaller vector sizes or use a scalar loop. It does not today, but I think we should guard against that potentially happening, otherwise bad things can happen.
> 
> I think the loop should exit when the last sub-range has been checked. We should rely on other tests to ensure the intrinsic method is operating efficiently.
> 
> 
> 2) This method only works when operating on byte arrays. It will not work correctly if operating on short or long arrays, since we are not adjusting the length and offsets accordingly by the scale. It's probably easier to just rename this as vectorizedMismatchLargeForBytes and drop the log2ArrayIndexScale argument. Then expand later if need be. I still think the method rightly belongs in ArraysSupport.

Sounds reasonable.

-Chris.

> Paul.
> 
>> On Jun 17, 2020, at 8:33 AM, Chris Hegarty <chris.hegarty at oracle.com> wrote:
>> 
>> The MemorySegment::mismatch implementation added vectorized mismatch of long sizes. The implementation is trivial, but the starting point for a more optimized implementation, if needed. ArraysSupport::vectorizedMismatchLarge incorrectly returns the bitwise complement of the offset of the first mismatch, where it should return the bitwise complement of the number of remaining pairs of elements to be checked in the tail of the two arrays. The AbstractMemorySegmentImpl::mismatch masked this problem, since it seamlessly compared the remaining tail, which is larger than it should be.
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~chegar/8247696/webrev.00/
>> 
>> I updated the exiting BulkOps micro-benchmark to cover mismatch. Here are the results, compared to ByteBuffer::mismatch, on my machine:
>> 
>> Benchmark                          Mode  Cnt        Score        Error  Units
>> BulkOps.mismatch_large_bytebuffer  avgt   30   740186.973 ? 119314.207  ns/op
>> BulkOps.mismatch_large_segment     avgt   30   683475.305 ?  76355.043  ns/op
>> BulkOps.mismatch_small_bytebuffer  avgt   30        7.367 ?      0.523  ns/op
>> BulkOps.mismatch_small_segment     avgt   30        4.140 ?      0.602  ns/op
>> 
>> 
>> -Chris.
>