RFR JDK-6321472: Add CRC-32C API
vitalyd at gmail.com
Fri Oct 17 19:51:54 UTC 2014
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>
> 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 =
> 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.
> 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
> >>> 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
> >> 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
> > 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
> >> 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
> >> 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
> > http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.01
> > Thanks,
> > Staffan
More information about the core-libs-dev