RFR JDK-6321472: Add CRC-32C API

[Staffan Friberg]

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 
> <stanimir at riflexo.com <mailto:stanimir at 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.friberg at oracle.com <mailto:staffan.friberg at 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
>     >
>     >
>
>