RFR JDK-6321472: Add CRC-32C API
Staffan Friberg
staffan.friberg at oracle.com
Thu Nov 6 10:18:15 UTC 2014
Anyone have time to be the second reviewer?
http://cr.openjdk.java.net/~sfriberg/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
>
> //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