[11] RFR 8193085 Vectorize the nio Buffer equals and compareTo implementations

[Paul Sandoz]

> On 18 Dec 2017, at 06:07, Alan Bateman <Alan.Bateman at 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.