Good point, looks like I was overly concerned about the null check and
inlining of that method. I had to manually inline the slicing-by-8 loop
to guarantee good performance.
Removed the ENDIAN field and use ByteOrder.nativeOrder() directly instead.
New webrev - http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.02
Thanks,
Staffan
On 10/17/2014 12:51 PM, Vitaly Davidovich wrote:
I wouldn't even bother with ENDIAN field; ByteOrder.nativeOrder(),
which calls Bits.byteOrder(), which returns a static final field
(modulo a null check) should get JIT compiled into just a read of
Bits.byteOrder. If storing ByteOrder.nativeOrder() in a static final
field actually makes a difference vs using ByteOrder.nativeOrder() in
JIT compiled code, I'd be pretty sad :).
On Fri, Oct 17, 2014 at 3:45 PM, Stanimir Simeonoff
<stani...@riflexo.com <mailto:stani...@riflexo.com>> wrote:
Hi,
I don't quite see the point of this line:
private transient final static ByteOrder ENDIAN
= ByteOrder.nativeOrder().equals(ByteOrder.BIG_ENDIAN)
? ByteOrder.BIG_ENDIAN : ByteOrder.LITTLE_ENDIAN;
should be just private static final ByteOrder ENDIAN =
ByteOrder.nativeOrder();
Also you don't need equals for enums alikes, just reference
comparison (==)
as they are constants.
Alternatively you can just replace it with a boolean as there are
no more
endianess options.
Stanimir
On Fri, Oct 17, 2014 at 9:58 PM, Staffan Friberg
<staffan.frib...@oracle.com <mailto:staffan.frib...@oracle.com>
> wrote:
> On 10/17/2014 04:05 AM, Alan Bateman wrote:
>
>> On 17/10/2014 02:42, Staffan Friberg wrote:
>>
>>> Hi,
>>>
>>> This RFE adds a CRC-32C class. It implements Checksum so it
will have
>>> the same API CRC-32, but use a different polynomial when
calculating the
>>> CRC checksum.
>>>
>>> CRC-32C implementation uses slicing-by-8 to achieve high
performance
>>> when calculating the CRC value.
>>>
>>> A part from adding the new class, java.util.zip.CRC32C, I have
also
>>> added two default methods to Checksum. These are methods that
were added to
>>> Adler32 and CRC32 in JDK 8 but before default methods were
added, which was
>>> why they were only added to the implementors and not the
interface.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-6321472
>>> Webrev:
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.00
<http://cr.openjdk.java.net/%7Esfriberg/JDK-6321472/webrev.00>
>>>
>>> I looked over the javadoc, I haven't found time to study the
>> implementation in CRC32C closely yet. Hopefully Sherman will be
able to
>> review as he I think he has prototyped several CRC32C
implementations.
>>
>> On Checksum#update(ByteBuffer) then a suggestion for:
>> "The checksum is updated using buffer.remaining, starting a
>> buffer.position"
>> is to replace it with:
>> "The checksum is updated with the remaining bytes in the buffer,
>> starting at the buffer's position."
>>
>
> Yes that reads much better. Updated CRC32 and Adler32 as well
since they
> have the same text.
>
>
>> In the @implNote then I wonder if it would be better to just
leave out
>> the note about when the invariants are broken, we don't do that
in other
>> places where breakage this is detected. Also I wonder if the
note about
>> "For best performance, ..." should be an @apiNote.
>>
> Done, removed the assert comment and changed the performance
note to an
> @apiNote.
>
>
>> Should CRC32C be final unless we have a good reason for it not
to be
>> final?
>>
> I simply followed what was used for CRC32 and Adler32, but I
don't see a
> reason for not making it final. Guess it is too late to make
those two
> classes final though?
>
>
>> In CRC32C then I assume you don't need the <p>/<p> for the first
>> statement. The @see Checksum might not be too interesting here
given that
>> it will be linked to anyway by way of implement Checksum.
>>
>
> Removed @see in all three classes.
>
>
>> I think it's more usual to list the @param tags before the @throws.
>>
> I removed that @param tags as they are already described in the
Checksum
> interface and will be picked up from there by Javadoc. Will do
the same in
> CRC32 and Adler32 as well.
>
>
>> For the bounds check then you might want to look at the wording
in other
>> areas (in java.lang or java.io <http://java.io> for example) to
get this better wording
>> (as off+len can overflow).
>>
> Done, I update CRC32 and Adler32 as well to make keep them as
similar as
> possible.
>
>
>> A minor comment on CRC32C is that you might want to keep the
line lengths
>> consistent with the rest of the code. I only mention is for future
>> side-by-side reviews to make it a bit easier and avoid too much
horizontal
>> scrolling.
>>
> Done, lines are now <80 or very close in a few cases where
breaking them
> made the code harder to read.
>
>>
>> -Alan
>>
>
> Here is a new webrev with the updates from Alan's and Peter's
suggestions.
> http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.01
<http://cr.openjdk.java.net/%7Esfriberg/JDK-6321472/webrev.01>
>
> Thanks,
> Staffan
>
>
