Thanks for comments. I have changed tests as Aleksey suggested. Btw. is not there some class used for utility methods as leftpad?
Ad performance of changed reverseBytes, I tried to compare both reverseBytes alone, and it seems to be same, but differs when inlined to reverse(). J. 2016-05-02 17:00 GMT+02:00 Claes Redestad <claes.redes...@oracle.com>: > On 2016-05-02 16:48, Aleksey Shipilev wrote: > >> On 05/02/2016 05:07 PM, Claes Redestad wrote: >> >>> I'd like to sponsor this patch proposed by Jaroslav Kameník here: >>> >>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/040660.html >>> >>> Bug https://bugs.openjdk.java.net/browse/JDK-8155795 >>> Webrev: http://cr.openjdk.java.net/~redestad/8155795/webrev.01/ >>> >> *) I wonder if Integer.reverseBytes is better spelled as: >> >> return (i >>> 24) | >> ((i & 0xFF0000) >> 8) | >> ((i & 0xFF00) << 8) | >> (i << 24); >> > > The reverseBytes changes were motivated by seeing slightly better > performance on the micro with the suggested changes, but after > discussing this a bit I think we should revert to the original code alone > and investigate if there's some compiler quirk lurking here separately. > > I'll file a bug. > > >> *) The test should probably follow the same randomized testing pattern >> as other tests: >> >> for (int i = 0; i < N; i++) { >> int x = rnd.nextInt(); >> >> String expected = new StringBuilder(). >> .append(leftpad(Integer.toBinaryString(x), 32)) >> .reverse().toString(); >> >> String actual = >> leftpad(Integer.toBinaryString(Integer.reverse(x)), 32); >> >> if (!expected.equals(actual)) { >> throw new RuntimeException("reverse: \n" + >> expected + " \n" + actual); >> } >> >> // That's how development looks like in 2016. >> private static String leftpad(String s, int width) { >> String r = s; >> for (int c = 0; c < width - s.length(); c++) { >> r = "0" + r; >> } >> return r; >> } >> >> ...and should probably precede any other test that uses reverse(). >> > > Seems reasonable. > > Jaroslav, do you mind improving the test as per Aleksey's > suggestions? > > Thanks! > > /Claes >
diff -r 2bf84670f079 src/java.base/share/classes/java/lang/Integer.java --- a/src/java.base/share/classes/java/lang/Integer.java Sat Apr 30 16:08:48 2016 -0700 +++ b/src/java.base/share/classes/java/lang/Integer.java Mon May 02 22:09:54 2016 +0200 @@ -1790,9 +1790,8 @@ i = (i & 0x55555555) << 1 | (i >>> 1) & 0x55555555; i = (i & 0x33333333) << 2 | (i >>> 2) & 0x33333333; i = (i & 0x0f0f0f0f) << 4 | (i >>> 4) & 0x0f0f0f0f; - i = (i << 24) | ((i & 0xff00) << 8) | - ((i >>> 8) & 0xff00) | (i >>> 24); - return i; + + return reverseBytes(i); } /** @@ -1820,10 +1819,10 @@ */ @HotSpotIntrinsicCandidate public static int reverseBytes(int i) { - return ((i >>> 24) ) | - ((i >> 8) & 0xFF00) | - ((i << 8) & 0xFF0000) | - ((i << 24)); + return (i << 24) | + ((i & 0xff00) << 8) | + ((i >>> 8) & 0xff00) | + (i >>> 24); } /** diff -r 2bf84670f079 src/java.base/share/classes/java/lang/Long.java --- a/src/java.base/share/classes/java/lang/Long.java Sat Apr 30 16:08:48 2016 -0700 +++ b/src/java.base/share/classes/java/lang/Long.java Mon May 02 22:09:54 2016 +0200 @@ -1952,10 +1952,8 @@ i = (i & 0x5555555555555555L) << 1 | (i >>> 1) & 0x5555555555555555L; i = (i & 0x3333333333333333L) << 2 | (i >>> 2) & 0x3333333333333333L; i = (i & 0x0f0f0f0f0f0f0f0fL) << 4 | (i >>> 4) & 0x0f0f0f0f0f0f0f0fL; - i = (i & 0x00ff00ff00ff00ffL) << 8 | (i >>> 8) & 0x00ff00ff00ff00ffL; - i = (i << 48) | ((i & 0xffff0000L) << 16) | - ((i >>> 16) & 0xffff0000L) | (i >>> 48); - return i; + + return reverseBytes(i); } /** diff -r 2bf84670f079 test/java/lang/Integer/BitTwiddle.java --- a/test/java/lang/Integer/BitTwiddle.java Sat Apr 30 16:08:48 2016 -0700 +++ b/test/java/lang/Integer/BitTwiddle.java Mon May 02 22:09:54 2016 +0200 @@ -58,6 +58,21 @@ for (int i = 0; i < N; i++) { int x = rnd.nextInt(); + + String expected = new StringBuilder() + .append(leftpad(toBinaryString(x), 32)) + .reverse().toString(); + + String actual = leftpad(toBinaryString(reverse(x)), 32); + + if (!expected.equals(actual)) { + throw new RuntimeException("reverse: \n" + + expected + " \n" + actual); + } + } + + for (int i = 0; i < N; i++) { + int x = rnd.nextInt(); if (highestOneBit(x) != reverse(lowestOneBit(reverse(x)))) throw new RuntimeException("g: " + toHexString(x)); } @@ -136,4 +151,12 @@ throw new RuntimeException("z: " + toHexString(x)); } } + + private static String leftpad(String s, int width) { + String r = s; + for (int c = 0; c < width - s.length(); c++) { + r = "0" + r; + } + return r; + } } diff -r 2bf84670f079 test/java/lang/Long/BitTwiddle.java --- a/test/java/lang/Long/BitTwiddle.java Sat Apr 30 16:08:48 2016 -0700 +++ b/test/java/lang/Long/BitTwiddle.java Mon May 02 22:09:54 2016 +0200 @@ -58,6 +58,21 @@ for (int i = 0; i < N; i++) { long x = rnd.nextLong(); + + String expected = new StringBuilder() + .append(leftpad(toBinaryString(x), 64)) + .reverse().toString(); + + String actual = leftpad(toBinaryString(reverse(x)), 64); + + if (!expected.equals(actual)) { + throw new RuntimeException("reverse: \n" + + expected + " \n" + actual); + } + } + + for (int i = 0; i < N; i++) { + long x = rnd.nextLong(); if (highestOneBit(x) != reverse(lowestOneBit(reverse(x)))) throw new RuntimeException("g: " + toHexString(x)); } @@ -136,4 +151,12 @@ throw new RuntimeException("z: " + toHexString(x)); } } + + private static String leftpad(String s, int width) { + String r = s; + for (int c = 0; c < width - s.length(); c++) { + r = "0" + r; + } + return r; + } }