Hello Staffan,
short question, the patch currently only adds the algorithm to be
used, is it planned/possible to add it as a supported algorithm to any
of the compressors or packers or is there no specification for that?
Besides that, the bound buffer now looks good.
Gruss
Bernd
Am Fri, 21 Nov
Hi David,
Thanks for reviewing.
CRC32C.java:
56 /**
57 * CRC-32C Polynom
58 */
“Polynomial”, perhaps?
Done
You did test this on both big- and little-endian boxes, right?
It looks right to me, but this stuff is brain-hurty on mirror-image dyslexia
and fenceposts.
Yes,
Hello,
Core libs changes (at this stage of JDK 9) only require a single reviewer.
Cheers,
-Joe
On 11/20/2014 4:45 AM, Staffan Friberg wrote:
Hi,
Anyone who can be the second Reviewer?
Thanks,
Staffan
On 11/06/2014 04:03 PM, Staffan Friberg wrote:
Hi Andrej,
Thanks for your comments. New
On 2014-11-20, at 7:45 AM, Staffan Friberg wrote:
> Hi,
>
> Anyone who can be the second Reviewer?
>
> Thanks,
> Staffan
I can review, but I am not a Reviewer.
CRC32C.java:
56 /**
57 * CRC-32C Polynom
58 */
“Polynomial”, perhaps?
You did test this on both big- and littl
Hi,
Anyone who can be the second Reviewer?
Thanks,
Staffan
On 11/06/2014 04:03 PM, Staffan Friberg wrote:
Hi Andrej,
Thanks for your comments. New webrev,
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.08
Indeed more common :)
jdk/src/java.base/share$ grep -R "private final stati
Hi Stanimir,
I believe this makes the code harder to follow as you now have to read
another method to follow the initialization of the arrays: The shifting
of crc and secondHalf is also different so it would still be some
difference between the two if I'm not mistaken.
Cheers,
Staffan
On 11
Hi Andrej,
Thanks for your comments. New webrev,
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.08
Indeed more common :)
jdk/src/java.base/share$ grep -R "private final static" *|wc -l
282
jdk/src/java.base/share$ grep -R "private static final" *|wc -l
3274
//Staffan
On 11/06/2014
Hi Staffan,
I'm not a reviewer but I have small remarks to the code style:
- the instance variable "crc" is declared bevor the class variables. I
would move it closer to the constructor.
- documentation comments should be used for the fields "crc" and
"CRC32C_POLY", e.g. instead of
// Calc
Hi Staffan,
Given the tables in the static class init :
66 private transient final static int[] byteTable0 = byteTables[0];
67 private transient final static int[] byteTable1 = byteTables[1];
68 private transient final static int[] byteTable2 = byteTables[2];
69 private tra
Anyone have time to be the second reviewer?
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
The CCC request has been approved for the new API.
Regards;
Staffan
On 10/22/2014 12:16 AM, Staffan Friberg wrote:
Thanks for the review. Updated the @implSpec.
Also includes Peter's good c
Hi,
Using unsafe to read fields will make the code very brittle and require
us to further detect and buffer types and which fields to read and
handle. Currently this seems to be a rather uncommon case, as the code
in Adler32 and CRC32 has done a full allocation in JDK 8, and by far
either wra
On 10/23/2014 10:16 AM, Stanimir Simeonoff wrote:
Unsafe is available, so the fields (array, offset) can be read directly
UNSAFE.getObject(buffer, hbOffset), UNSAFE.getObject(buffer, offsetOffset).
No need for MethodHandlers.
During class init the offsets have to be resolved, pretty much like any
>>, UNSAFE.getObject(buffer, offsetOffset)
Obviously should be Unsafe.getInt(buffer, offsetOffset)
On Thu, Oct 23, 2014 at 11:16 AM, Stanimir Simeonoff
wrote:
> Unsafe is available, so the fields (array, offset) can be read directly
> UNSAFE.getObject(buffer, hbOffset), UNSAFE.getObject(buffer,
Unsafe is available, so the fields (array, offset) can be read directly
UNSAFE.getObject(buffer, hbOffset), UNSAFE.getObject(buffer, offsetOffset).
No need for MethodHandlers.
During class init the offsets have to be resolved, pretty much like any CAS
utilizing algorithm.
I didn't propose it as re
On 10/23/2014 03:52 AM, Staffan Friberg wrote:
Webrev with these last updates. Added more tests to make sure CRC32C,
CRC32 and Checksum default methods all are covered.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
Hi Staffan,
Regarding default case:
168 } else {
Webrev with these last updates. Added more tests to make sure CRC32C,
CRC32 and Checksum default methods all are covered.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
//Staffan
On 10/22/2014 05:37 PM, Stanimir Simeonoff wrote:
Hi Staffan,
The readonly buffer (ByteBuffer.asR
Hi Staffan,
The readonly buffer (ByteBuffer.asReadOnlyBuffer()) don't have array()
"working".
You can use "int length = Math.min(buffer.remaining, b.length)" instead,
same with new byte[Math.min(4096, buffer.remaining)]. Using smaller chunks
will be more performance friendly than allocating/eatin
Just realized that in the Checksum default case we will actually end up
there for Direct buffers.
//Staffan
On 10/22/2014 05:06 PM, Staffan Friberg wrote:
Hi,
I was thinking about this earlier when I started writing the patch and
then I forgot about it again. I haven't been able to figure ou
Hi,
I was thinking about this earlier when I started writing the patch and
then I forgot about it again. I haven't been able to figure out when the
code will be executed. ByteBuffer is implemented in such a way that
only the JDK can extend it and as far as I can tell you can only create
3 ty
On Thu, Oct 23, 2014 at 12:10 AM, Bernd Eckenfels
wrote:
> Hello,
>
> just a question in the default impl:
>
> +} else {
> +byte[] b = new byte[rem];
> +buffer.get(b);
> +update(b, 0, b.length);
> +}
>
> would it be a good idea to actually put a
Hello,
just a question in the default impl:
+} else {
+byte[] b = new byte[rem];
+buffer.get(b);
+update(b, 0, b.length);
+}
would it be a good idea to actually put a ceiling on the size of the
array which is processed at once? Something below
You're right, managed to convince myself here as well. Will change it to <=.
An unfortunate side-effect seems to be that in a benchmark with 1024
long array it seems like the performance drops when the tail loop is not
utilized.
As you can see below there is a slight drop when the data is perfe
On 10/21/2014 11:34 PM, Staffan Friberg wrote:
I believe it must be <, as it is in the tail loop as well, because end
is (off+len or limit) so end is exclusive, similar to
subString(begin,end).
Makes sense?
//Staffan
On 10/21/2014 01:46 PM, Peter Levart wrote:
Sorry Staffan, another nit...
Thanks for the review. Updated the @implSpec.
Also includes Peter's good catch.
Webrev: http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.06
//Staffan
On 10/21/2014 02:09 PM, Xueming Shen wrote:
Staffan,
Thanks for the package.html update.
Just wonder if it would be better to use
I believe it must be <, as it is in the tail loop as well, because end
is (off+len or limit) so end is exclusive, similar to subString(begin,end).
Makes sense?
//Staffan
On 10/21/2014 01:46 PM, Peter Levart wrote:
Sorry Staffan, another nit...
212 for (; off < (end - Long.BYTES
Hi Peter,
Thanks for catching!
I would like to keep to 8 since the crc32c instruction on SPARC will
require it, the plan is to add an intrinsic for it.
Thanks,
Staffan
On 10/21/2014 01:30 PM, Peter Levart wrote:
On 10/21/2014 08:49 PM, Staffan Friberg wrote:
Hi,
Got an offline comment th
Staffan,
Thanks for the package.html update.
Just wonder if it would be better to use
buffer.remaining(), instead of the buffer.limit() - buffer.position()
in Checksum.udpate(ByteBuffer)'s #implSpec
The rest looks fine for me.
-Sherman
On 10/21/2014 01:11 PM, Staffan Friberg wrote:
Con
On 10/21/2014 01:11 PM, Staffan Friberg wrote:
Converted.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.05
Conversion looks fine; thanks,
-Joe
//Staffan
On 10/21/2014 12:34 PM, Joe Darcy wrote:
Hi Staffan,
If you are updating package.html, please also hg mv the file to be a
p
Sorry Staffan, another nit...
212 for (; off < (end - Long.BYTES); off += Long.BYTES) {
and
286 for (; off < (end - Long.BYTES); off += Long.BYTES) {
I think you could change "off < (end - Long.BYTES)" to "off <= (end -
Long.BYTES)". Am I right?
Regards, Peter
On 10/21/2014 08:49 PM, Staffan Friberg wrote:
Hi,
Got an offline comment that the package.html should be update as well
to cover CRC-32C.
Otherwise there are no code changes in this new webrev.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.04
Hi Staffan,
198 if (end
Converted.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.05
//Staffan
On 10/21/2014 12:34 PM, Joe Darcy wrote:
Hi Staffan,
If you are updating package.html, please also hg mv the file to be a
package-info.java file with the equivalent javadoc.
Thanks,
-Joe
On 10/21/2014 11:49 A
Hi Staffan,
If you are updating package.html, please also hg mv the file to be a
package-info.java file with the equivalent javadoc.
Thanks,
-Joe
On 10/21/2014 11:49 AM, Staffan Friberg wrote:
Hi,
Got an offline comment that the package.html should be update as well
to cover CRC-32C.
Ot
Hi,
Got an offline comment that the package.html should be update as well to
cover CRC-32C.
Otherwise there are no code changes in this new webrev.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.04
//Staffan
On 10/21/2014 10:28 AM, Staffan Friberg wrote:
Hi Peter,
Thanks for the
Hi Peter,
Thanks for the comments..
217 if (Unsafe.ADDRESS_SIZE == 4) {
218 // On 32 bit platforms read two ints instead of a
single 64bit long
When you're reading from byte[] using Unsafe (updateBytes), you have
the option of reading 64bit values on 6
Hi David,
The intrinsic that can be implemented for CRC32C will probably be even faster
as it can make use of the specific crc32c instruction instead of using carry
less multiplication.
It might not hurt to mock up a quick benchmark using the CRC32C
instruction against jdk8’s java.util.zip.CR
On 2014-10-17, at 5:22 PM, Staffan Friberg wrote:
>
> The polynomial used for multiplication is different so while, as with all
> CRCs, the algorithm is similar, but a different polynomial is used and the
> final checksum will be different.
Yes, I was misled by the comments. Wikipedia makes
Am 18.10.2014 um 00:38 schrieb Staffan Friberg:
Hi Ulf,
Since the default method only calls a single method it will most likely be
inlined.
After inlining the check will be
if (0 < 0 || b.length < 0 || 0 > b.length - b.length) {
throw new ArrayIndexOutOfBoundsException();
On 10/17/2014 08:58 PM, Staffan Friberg wrote:
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
Hi Staffan,
A few more comments...
217 if (Unsafe.ADDRESS_SIZE == 4) {
218
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/~sfri
Hi Ulf,
Since the default method only calls a single method it will most likely
be inlined.
After inlining the check will be
if (0 < 0 || b.length < 0 || 0 > b.length - b.length) {
throw new ArrayIndexOutOfBoundsException();
}
Which will be optimized away so only
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.frib...@oracle.com> wrote:
> Hi Stanimir,
>
> Unsafe.ADDRESS_SIZE is the size of a native pointer, and is not aff
Hi David,
Not sure what you mean with a bit flip. Calculating CRC32 and then
flipping the result in some way?
The polynomial used for multiplication is different so while, as with
all CRCs, the algorithm is similar, but a different polynomial is used
and the final checksum will be different.
See also: http://cr.openjdk.java.net/~drchase/7088419/webrev.01/
This is the version that was not coded as an intrinsic, that also included
fork/join. The crazy Intel instructions are accessed from native code,
so you can get a feel for what the code looks like before it is converted
to an intri
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: " +
On 2014-10-17, at 2:53 PM, Staffan Friberg wrote:
> Fully agree that using Unsafe makes one sad.
>
> I'm just about to send out a new webrev with Alan's and Peter's comments,
> once I have that done I will give using the NIO-Buffer API a second try to
> see if using IntBuffer and LongBuffer i
Am 17.10.2014 um 20:58 schrieb Staffan Friberg:
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
I would not remove:
86 public void update(byte[] b) {
87 adler = updateBytes(adler, b, 0, b.l
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 bool
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
differen
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 =
ByteOrde
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 t
Fully agree that using Unsafe makes one sad.
I'm just about to send out a new webrev with Alan's and Peter's
comments, once I have that done I will give using the NIO-Buffer API a
second try to see if using IntBuffer and LongBuffer is able to achieve
similar performance.
As I noted in my rep
Actually I was under the impression I had included the list. Getting it
done w now.
Overall if you want speed you go unsafe (bit sad) as the JIT may not remove
the bound checks off the ByteBuffer. Unsafe is pretty ugly overall, though
and personally I try to avoid it giving up performance.
On 64b
Hi Staffan,
You're right. I thought ByteBuffer was more optimal in this respect.
Regards, Peter
On 10/17/2014 06:47 PM, Staffan Friberg wrote:
Hi Peter,
Thanks for reviewing.
I have switched to the Integer methods. Was looking through that API
but I was too stuck with the reflect and swap
On 10/17/2014 01:46 AM, Peter Levart wrote:
On 10/17/2014 03:42 AM, 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
In my experiments, ByteBuffer.wrap() with immediate use (e.g. BB.wrap
(...).getInt (...)) does not trigger escape analysis (I.e. the BB is not
eliminated). I suspect it's because wrap () is not trivial enough and
compiler doesn't "see through" the noise there.
Sent from my phone
On Oct 17, 2014 4
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
On 10/17/2014 03:42 AM, 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 calculat
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 c
58 matches
Mail list logo
