Thanks for looking at this Chris
On 17/06/2020 21:56, Paul Sandoz 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?
I suggested that to Chris, since sometimes asserts are enabled when
running tests, sometimes are not - in langtools we moved away from using
asserts as we realized that in certain cases they were silently failing.
I'm ok with whatever standard you feel comfortable with though.
—
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. 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.
So your worry here is that we'll end up with an infinite loop, right?
If so, we could check remaining against previous remaining and bomb out
too if no further progress seem to be made?
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.
Yep - probably good idea to restrict on bytes, for now.
Maurizio
Paul.
On Jun 17, 2020, at 8:33 AM, Chris Hegarty <chris.hega...@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.
