RFR JDK-6321472: Add CRC-32C API
Staffan Friberg
staffan.friberg at oracle.com
Thu Nov 6 15:12:35 UTC 2014
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.friberg at oracle.com <mailto:staffan.friberg at 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
>
>
>
>
>
>
>
>
More information about the core-libs-dev
mailing list