RFR JDK-6321472: Add CRC-32C API

[Stanimir Simeonoff]

Thanks Staffan,

You're absolutely right. Funny enough I was absolutely sure compressed ops
affected it.

Thanks
Stanimir


On Sat, Oct 18, 2014 at 12:00 AM, Staffan Friberg <
staffan.friberg at oracle.com> wrote:

>  Hi Stanimir,
>
> Unsafe.ADDRESS_SIZE is the size of a native pointer, and is not affected
> by how the JVM handles Java references on the heap.
>
> --------------
>
> import sun.misc.Unsafe;
>
> public class AddressSize {
>
>    public static void main(String[] args) {
>       System.out.println("Address Size: " + Unsafe.ADDRESS_SIZE);
>    }
> }
>
> --------------
>
>
> $ java -showversion -XX:-UseCompressedOops AddressSize
> java version "1.8.0_25"
> Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
> Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)
>
> Address Size: 8
>
>
> $ java -showversion -XX:+UseCompressedOops AddressSize
> java version "1.8.0_25"
> Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
> Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)
>
> Address Size: 8
>
>
> $ ./jdk1.8.0_20-b26_32bit/bin/java -showversion AddressSize
> java version "1.8.0_20"
> Java(TM) SE Runtime Environment (build 1.8.0_20-b26)
> Java HotSpot(TM) Server VM (build 25.20-b23, mixed mode)
>
> Address Size: 4
>
> //Staffan
>
>
> On 10/17/2014 12:54 PM, Stanimir Simeonoff wrote:
>
> Also, ede
>
> Unsafe.ADDRESS_SIZE == 4
>
> That doesn't imply 32bit systems as with less than 32GiB (on 64bit) the
> default is using compressed options and the address size is still only
> 4bytes.
>  I usually use something like this (in user space code) to detect the
> architecture
>     private static final boolean is64Bit;
>     static{
>         final String p =  java.security.AccessController.doPrivileged(new
> PrivilegedAction<String>() {
>             @Override
>             public String run() {
>                 return System.getProperty("os.arch",
> "x64")+System.getProperty("sun.arch.data.model"", ");
>             }
>         });
>         is64Bit = p.indexOf("64")>=0;
>     };
>
>  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
>>
>>
>
>