> On 18 Dec 2017, at 06:07, Alan Bateman <alan.bate...@oracle.com> wrote:
>
> On 15/12/2017 20:29, Paul Sandoz wrote:
>> Hi,
>>
>> Please review this patch to vectorize the buffer equals and compareTo
>> implementations, using the same approach that was used for arrays:
>>
>>
>> http://cr.openjdk.java.net/~psandoz/jdk/JDK-8193085-buffer-equals-compareTo-vectorize/webrev/
>>
>> <http://cr.openjdk.java.net/~psandoz/jdk/JDK-8193085-buffer-equals-compareTo-vectorize/webrev/>
>>
>> This patch expands on using the double address mode of unsafe to uniformly
>> access a memory region covered by a buffer be it on or off heap. Only
>> buffers with the same endianness can support optimized equality and
>> comparison.
> I've looked through the changes and it looks quite good.
>
> It might be useful to add a comment in StringCharBuffer to explain why
> equals/compareTo are overridden.
See below.
> Also it might be safer if the mismatch method that takes CharBuffers checks
> if one of charRegionOrders is null to guarantee that it will never do a
> vectorizedMismatch test with a wrapped char sequence (it would catch a
> serious bug if someone were to misuse to call it with two StringCharBuffers).
There is already an assert, perhaps i can simplify this:
1) StringCharBuffer does not require special overrides.
2) Update the mismatch method:
static int mismatch(CharBuffer a, int aOff, CharBuffer b, int bOff, int length)
{
int i = 0;
// Ensure only heap or off-heap buffer instances use the
// vectorized mismatch. If either buffer is a StringCharBuffer
// (order is null) then the slow path is taken
if (length > 3 && a.charRegionOrder() == b.charRegionOrder()
&& a.charRegionOrder() != null && b.charRegionOrder() != null) {
I updated the webrev in place (i also updated the test to test big vs. little
endian).
> Not for this patch but XXXBuffer.compareTo doesn't specify how it compares
> when the remaining elements in one buffer is a proper prefix of the remaining
> elements in the other buffer.
>
Right. There is a follow on bug to add new API points and we can do a cleanup
as part of that.
> It's hard to see the changes to ArraySupport. I assume it's just the package
> name and changing the methods to public. I can't tell why webrev doesn't
> handle the move in this case.
I dunno why that was not tracked. It’s just changes to the package name and
method accessibility.
> Another one is Arrays where they are some re-formatting in the patch that
> webrev doesn't show (I can't tell if this is intended or not).
>
That was a refactoring glitch, fixed:
http://cr.openjdk.java.net/~psandoz/jdk/JDK-8193085-buffer-equals-compareTo-vectorize/webrev/src/java.base/share/classes/java/util/Arrays.java.sdiff.html
> It would be good if the really long lines in BufferMismatch could be trimmed
> down (maybe import static ArraySupport) as it will very annoying to do
> side-by-side diffs when reviewing future changes.
Done.
> There are several places where the styles differs to the style in this area
> but it's probably not worth spending time on.
>
Agreed, the only sane way to do this is to have auto-formatting on commit (i
have it set up in my IDE but it’s obviously not consistent will all source
code). And it requires a benevolent dictator with good taste to state the
format, since i suspect we will never reach consensus on such matters :-)
> The test looks okay although it overlaps with the existing tests.
>
Yes, although the existing Buffer equals/compareTo tests are somewhat limited
(especially regarding length). These new tests will also help if/when a public
mismatch method is added.
Thanks,
Paul.
