RFR JDK-6321472: Add CRC-32C API
Staffan Friberg
staffan.friberg at oracle.com
Tue Oct 21 22:16:04 UTC 2014
Thanks for the review. Updated the @implSpec.
Also includes Peter's good catch.
Webrev: http://cr.openjdk.java.net/~sfriberg/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
>>
>> //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
>>>>
>>>> //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
>>>>>
>>>>> Cheers,
>>>>> Staffan
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list