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

Paul Sandoz paul.sandoz at oracle.com
Mon Dec 18 20:55:51 UTC 2017

> 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:


> 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.


> 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.


More information about the core-libs-dev mailing list