Hi Stanimir,
I believe this makes the code harder to follow as you now have to read
another method to follow the initialization of the arrays: The shifting
of crc and secondHalf is also different so it would still be some
difference between the two if I'm not mistaken.
Cheers,
Staffan
On 11/06/2014 12:26 PM, Stanimir Simeonoff wrote:
Hi Staffan,
Given the tables in the static class init :
66 private transient final static int[] byteTable0 = byteTables[0];
67 private transient final static int[] byteTable1 = byteTables[1];
68 private transient final static int[] byteTable2 = byteTables[2];
69 private transient final static int[] byteTable3 = byteTables[3];
70 private transient final static int[] byteTable4 = byteTables[4];
71 private transient final static int[] byteTable5 = byteTables[5];
72 private transient final static int[] byteTable6 = byteTables[6];
73 private transient final static int[] byteTable7 = byteTables[7];
I was wondering if assigning the tables in the static part in similar fashion
would make the code much more readable:
66 private transient final static int[] byteTable0 = byteTables[idx(0)];
67 private transient final static int[] byteTable1 = byteTables[idx(1)];
68 private transient final static int[] byteTable2 = byteTables[idx(2)];
69 private transient final static int[] byteTable3 = byteTables[idx(3)];
70 private transient final static int[] byteTable4 = byteTables[idx(4(];
71 private transient final static int[] byteTable5 = byteTables[idx(5)];
72 private transient final static int[] byteTable6 = byteTables[idx(6)];
73 private transient final static int[] byteTable7 = byteTables[idx(7)];
private static int idx(int i){ return ByteOrder.nativeOrder() ==
ByteOrder.LITTLE_ENDIAN?7-i:i;};
Then in the loops you can skip checking the condition, something like:
296- if (ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN) {
297- crc = byteTable7[crc & 0xFF]
298- ^ byteTable6[(crc >>> 8) & 0xFF]
299- ^ byteTable5[(crc >>> 16) & 0xFF]
300- ^ byteTable4[crc >>> 24]
301- ^ byteTable3[secondHalf & 0xFF]
302- ^ byteTable2[(secondHalf >>> 8) & 0xFF]
303- ^ byteTable1[(secondHalf >>> 16) & 0xFF]
304- ^ byteTable0[secondHalf >>> 24];
305- } else { // ByteOrder.BIG_ENDIAN
306 crc = byteTable0[secondHalf & 0xFF]
307 ^ byteTable1[(secondHalf >>> 8) & 0xFF]
308 ^ byteTable2[(secondHalf >>> 16) & 0xFF]
309 ^ byteTable3[secondHalf >>> 24]
310 ^ byteTable4[crc & 0xFF]
311 ^ byteTable5[(crc >>> 8) & 0xFF]
312 ^ byteTable6[(crc >>> 16) & 0xFF]
313 ^ byteTable7[crc >>> 24];
314 - }
Since the byteTabeleN are properly assigned during the class init based on the
endianess of the systen. Of course, there won't be any performance benefits
since hotspot should constant fold the code
but it'll be easier to maintain and read.
Cheers
Stanimir
On Thu, Nov 6, 2014 at 12:18 PM, Staffan Friberg
<staffan.frib...@oracle.com <mailto:staffan.frib...@oracle.com>> wrote:
Anyone have time to be the second reviewer?
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
<http://cr.openjdk.java.net/%7Esfriberg/JDK-6321472/webrev.07>
The CCC request has been approved for the new API.
Regards;
Staffan
On 10/22/2014 12:16 AM, Staffan Friberg wrote:
Thanks for the review. Updated the @implSpec.
Also includes Peter's good catch.
Webrev:
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.06
<http://cr.openjdk.java.net/%7Esfriberg/JDK-6321472/webrev.06>
//Staffan
On 10/21/2014 02:09 PM, Xueming Shen wrote:
Staffan,
Thanks for the package.html update.
Just wonder if it would be better to use
buffer.remaining(), instead of the buffer.limit() -
buffer.position()
in Checksum.udpate(ByteBuffer)'s #implSpec
The rest looks fine for me.
-Sherman
On 10/21/2014 01:11 PM, Staffan Friberg wrote:
Converted.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.05
<http://cr.openjdk.java.net/%7Esfriberg/JDK-6321472/webrev.05>
//Staffan
On 10/21/2014 12:34 PM, Joe Darcy wrote:
Hi Staffan,
If you are updating package.html, please also hg
mv the file to be a package-info.java file with
the equivalent javadoc.
Thanks,
-Joe
On 10/21/2014 11:49 AM, Staffan Friberg wrote:
Hi,
Got an offline comment that the package.html
should be update as well to cover CRC-32C.
Otherwise there are no code changes in this
new webrev.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.04
<http://cr.openjdk.java.net/%7Esfriberg/JDK-6321472/webrev.04>
//Staffan
On 10/21/2014 10:28 AM, Staffan Friberg wrote:
Hi Peter,
Thanks for the comments..
217 if
(Unsafe.ADDRESS_SIZE == 4) {
218 // On 32 bit
platforms read two ints instead of a
single 64bit long
When you're reading from byte[] using
Unsafe (updateBytes), you have the
option of reading 64bit values on
64bit platforms. When you're reading
from DirectByteBuffer memory
(updateDirectByteBuffer), you're only
using 32bit reads.
I will add a comment in the code for this
decision. The reason is that read a long
results in slightly worse performance in
this case, in updateBytes it is faster. I
was able to get it to run slightly faster
by working directly with the address
instead of always adding address + off,
but this makes things worse in the 32bit
case since all calculation will now be
using long variables. So using the getInt
as in the current code feels like the best
solution as it strikes the best balance
between 32 and 64bit. Below is how
updateByteBuffer looked with the rewrite I
mentioned.
ong address = ((DirectBuffer)
buffer).address();
crc = updateDirectByteBuffer(crc, address
+ pos, address + limit);
private static int
updateDirectByteBuffer(int crc, long adr,
long end) {
// Do only byte reads for arrays
so short they can't be aligned
if (end - adr >= 8) {
// align on 8 bytes
int alignLength = (8 - (int)
(adr & 0x7)) & 0x7;
for (long alignEnd = adr +
alignLength; adr < alignEnd; adr++) {
crc = (crc >>> 8)
^ byteTable[(crc ^
UNSAFE.getByte(adr)) & 0xFF];
}
if (ByteOrder.nativeOrder() ==
ByteOrder.BIG_ENDIAN) {
crc =
Integer.reverseBytes(crc);
}
// slicing-by-8
for (; adr < (end -
Long.BYTES); adr += Long.BYTES) {
int firstHalf;
int secondHalf;
if (Unsafe.ADDRESS_SIZE ==
4) {
// On 32 bit platforms
read two ints instead of a single 64bit long
firstHalf =
UNSAFE.getInt(adr);
secondHalf =
UNSAFE.getInt(adr + Integer.BYTES);
} else {
long value =
UNSAFE.getLong(adr);
if
(ByteOrder.nativeOrder() ==
ByteOrder.LITTLE_ENDIAN) {
firstHalf = (int)
value;
secondHalf = (int)
(value >>> 32);
} else { //
ByteOrder.BIG_ENDIAN
firstHalf = (int)
(value >>> 32);
secondHalf = (int)
value;
}
}
crc ^= firstHalf;
if
(ByteOrder.nativeOrder() ==
ByteOrder.LITTLE_ENDIAN) {
crc = byteTable7[crc &
0xFF]
^
byteTable6[(crc >>> 8) & 0xFF]
^
byteTable5[(crc >>> 16) & 0xFF]
^
byteTable4[crc >>> 24]
^
byteTable3[secondHalf & 0xFF]
^
byteTable2[(secondHalf >>> 8) & 0xFF]
^
byteTable1[(secondHalf >>> 16) & 0xFF]
^
byteTable0[secondHalf >>> 24];
} else { //
ByteOrder.BIG_ENDIAN
crc =
byteTable0[secondHalf & 0xFF]
^
byteTable1[(secondHalf >>> 8) & 0xFF]
^
byteTable2[(secondHalf >>> 16) & 0xFF]
^
byteTable3[secondHalf >>> 24]
^
byteTable4[crc & 0xFF]
^
byteTable5[(crc >>> 8) & 0xFF]
^
byteTable6[(crc >>> 16) & 0xFF]
^
byteTable7[crc >>> 24];
}
}
if (ByteOrder.nativeOrder() ==
ByteOrder.BIG_ENDIAN) {
crc =
Integer.reverseBytes(crc);
}
}
// Tail
for (; adr < end; adr++) {
crc = (crc >>> 8)
^ byteTable[(crc ^
UNSAFE.getByte(adr)) & 0xFF];
}
return crc;
}
Also, in updateBytes, the usage of
Unsafe.ARRAY_INT_INDEX_SCALE/ARRAY_LONG_INDEX_SCALE
to index a byte array sounds a little
scary. To be ultra portable you could
check that ARRAY_BYTE_INDEX_SCALE == 1
first and refuse to use Unsafe for
byte arrays if it is not 1. Then use
Integer.BYTES/Long.BYTES to manipulate
'offsets' instead. In
updateDirectByteBuffer it would be
more appropriate to use
Integer.BYTES/Long.BYTES too.
Good idea. Added a check in the initial if
statement and it will get automatically
optimized away.
225 firstHalf = (int) (value &
0xFFFFFFFF);
226
secondHalf = (int) (value >>> 32);
227 } else { //
ByteOrder.BIG_ENDIAN
228
firstHalf = (int) (value >>> 32);
229
secondHalf = (int) (value & 0xFFFFFFFF);
firstHalf = (int) value; // this is
equivalent for line 225
secondHalf = (int) value; // this is
equivalent for line 229
Done.
Here is the latest webrev,
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.03
<http://cr.openjdk.java.net/%7Esfriberg/JDK-6321472/webrev.03>
Cheers,
Staffan
