On 15 Dec 2017, at 13:01, David Lloyd <david.lloyd@redhat.com> wrote:

I'm not a reviewer, but I was curious about this change; unfortunately the diff seems to be dominated by case and formatting changes making the actual functional aspect change hard to divine.

If not already i recommend viewing via udiffs, i find that makes it easier.

Within the JBoss unit we have an informal policy that formatting changes should be presented separately so that it's easier to trace back problems in the future, as well as being much easier to review the change in the first place. Would I be stepping out of bounds to suggest that this change should be similarly divided?

In hindsight :-) at this point i would prefer not to split it unless reviewers are having a really hard time. I was furiously hacking on this and got fed up with the names making it harder for me to reason about the code so i changed ‘em mid-flight when doing this work. Thanks, Paul.

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

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.

On 19 Dec 2017, at 11:35, Alan Bateman <Alan.Bateman@oracle.com> wrote:

On 18/12/2017 20:55, Paul Sandoz wrote:

...

: 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). When I looked at it yesterday the CharBuffer version of mismatch wasn't checking both bases (or maybe I just mis-read it).

You did not misread it. Previously it strictly did not need to check both (only argument a) because the StringCharBuffer equals/compareTo did not call the mismatch method.

Anyway, it looks good now and I see the other bug updated to list clarifying the compareTo specs too.

Thanks, Paul.