RFR JDK-6321472: Add CRC-32C API

[Vitaly Davidovich]

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>
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
> > 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
> >>>
> >>>  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 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
> >
> > Thanks,
> > Staffan
> >
> >
>