Using unsigned library work in the JDK - please rename them!
Ulf Zibis
Ulf.Zibis at gmx.de
Tue Feb 7 15:23:32 UTC 2012
Hi again,
isn' there any chance for a renaming?
I think, e.g.,
+ return unsigned(b[n]) | (unsigned(b[n + 1]) << 8);
would be much more writeable + readable than ...
+ return Byte.toUnsignedInt(b[n]) | (Byte.toUnsignedInt(b[n + 1]) << 8);
In 2nd case, static import is not possible, as we have:
- int Byte.toUnsignedInt(byte)
- int Short.toUnsignedInt(short)
but would be very easy, if we only would have:
- short Short.unsigned(byte)
- int Integer.unsigned(short)
- long Long.unsigned(int)
- BigInteger BigInteger.unsigned(long)
Nothing would be ambiguous, using static import + we should not use casting to get an unsigned short
from a byte.
Sherman, could you do your performance test, using instead "int Byte.toUnsignedInt(byte)"? :
public class Short {
public short unsigned(byte b) {
return b & 0xFF;
}
-Ulf
Am 07.02.2012 06:33, schrieb Joe Darcy:
> Thanks Sherman; given the at most marginal performance cost, I'll push the changes in the next day
> or two.
>
> -Joe
>
>
> On 02/06/2012 05:02 PM, Xueming Shen wrote:
>> Joe,
>>
>> I did some non-scientific measure of the performance. test1() is to measure the change
>> in a real "listing" operation on a ZipInputStream and test2() is a pure access on the byte[]
>> directly. I don't see any significant difference on performance in both -client and -server
>> vm. I would assume the change has no performance impact to the real zip access.
>>
>> In -client vm
>> test1: 108 vs 109
>> test2: 280 : 283
>>
>> Simple test case attached (the input is a big enough jar file, such as the rt.jar)
>>
>> -Sherman
>>
>> --------------------------------------------
>>
>> public static void main(String[] args) throws IOException {
>>
>> byte[] data = java.nio.file.Files.readAllBytes(new File(args[0]).toPath());
>> ByteArrayInputStream bais = new ByteArrayInputStream(data);
>>
>> //warm up
>> test1(bais, 100);
>> System.out.printf("t=%d%n", test1(bais, 500));
>>
>> test2(data, 100);
>> System.out.printf("t=%d%n", test2(data, 100));
>> }
>>
>> static long test1(ByteArrayInputStream bais, int n) throws IOException {
>> long t = 0;
>> for (int i = 0; i < n; i++) {
>> bais.reset();
>>
>> ZipInputStream zis = new ZipInputStream(bais);
>> ZipEntry e;
>> long t0 = System.currentTimeMillis();
>> while ((e = zis.getNextEntry()) != null) {
>> zis.closeEntry();
>> //System.out.println(new String(e.getName()));
>> }
>> long t1 = System.currentTimeMillis();
>> t += t1 - t0;
>> }
>> return t /= n;
>> }
>>
>>
>> static long test2(byte data[], int n) throws IOException {
>> int j = 10000;
>> long t = 0;
>> if (j >= data.length -1)
>> j = data.length -2;
>> for (int m = 0; m < n; m++) {
>> long t0 = System.nanoTime();
>> int sum = 0;
>> for (int i = 0; i < j; i++) {
>> sum += get16(data, i);
>> }
>> long t1 = System.nanoTime();
>> t += t1 - t0;
>> }
>> return t / n / 100;
>> }
>>
>> private static final int get16(byte b[], int off) {
>> return (b[off] & 0xff) | ((b[off+1] & 0xff) << 8);
>> //return Byte.toUnsignedInt(b[off]) | (Byte.toUnsignedInt(b[off+1]) << 8);
>>
>> }
>>
>>
>> On 1/26/2012 5:33 PM, Joe Darcy wrote:
>>> Sherman, thanks for offering to investigate the performance impact, if any, of this change.
>>>
>>> -Joe
>>>
>>> PS All the jar/zip regression tests pass with this change.
>>>
>>> On 01/26/2012 09:52 AM, Xueming Shen wrote:
>>>> Joe,
>>>>
>>>> The changes look fine. However I have the same performance concern in
>>>> cases that the vm fails to inline the toUnsignedInt(), especially for those
>>>> "simple" one line utility methods, such as the get16(), CH(), SH(), my
>>>> guess is that it might have a performance impact. It might not be a big
>>>> deal for operation like creating or extracting a jar/zip file but probably will
>>>> slow down the loc/cen table reading only operation such as list a jar/zip
>>>> file. I will try to get some performance data to see if the impact is
>>>> "significant" enough to be a real concern.
>>>>
>>>> -Sherman
>>>>
>>>> On 01/25/2012 08:37 PM, Joe Darcy wrote:
>>>>> Hello,
>>>>>
>>>>> As a follow-up to the recent push of unsigned library support in the JDK [1], I grepped -i for
>>>>> "0xff" in the JDK code base to look for candidate locations to use the new methods. I choose
>>>>> to first tackle some jar/zip code.
>>>>>
>>>>> Sherman, can you review the changes below?
>>>>>
>>>>> diff -r 303b67074666 src/share/classes/java/util/jar/JarOutputStream.java
>>>>> --- a/src/share/classes/java/util/jar/JarOutputStream.java Tue Jan 24 15:13:27 2012 -0500
>>>>> +++ b/src/share/classes/java/util/jar/JarOutputStream.java Wed Jan 25 20:31:05 2012 -0800
>>>>> @@ -1,5 +1,5 @@
>>>>> /*
>>>>> - * Copyright (c) 1997, 2006, Oracle and/or its affiliates. All rights reserved.
>>>>> + * Copyright (c) 1997, 2012, Oracle and/or its affiliates. All rights reserved.
>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>> *
>>>>> * This code is free software; you can redistribute it and/or modify it
>>>>> @@ -135,7 +135,7 @@
>>>>> * The bytes are assumed to be in Intel (little-endian) byte order.
>>>>> */
>>>>> private static int get16(byte[] b, int off) {
>>>>> - return (b[off] & 0xff) | ((b[off+1] & 0xff) << 8);
>>>>> + return Byte.toUnsignedInt(b[off]) | ( Byte.toUnsignedInt(b[off+1]) << 8);
>>>>> }
>>>>>
>>>>> /*
>>>>> diff -r 303b67074666 src/share/classes/java/util/jar/Manifest.java
>>>>> --- a/src/share/classes/java/util/jar/Manifest.java Tue Jan 24 15:13:27 2012 -0500
>>>>> +++ b/src/share/classes/java/util/jar/Manifest.java Wed Jan 25 20:31:05 2012 -0800
>>>>> @@ -1,5 +1,5 @@
>>>>> /*
>>>>> - * Copyright (c) 1997, 2006, Oracle and/or its affiliates. All rights reserved.
>>>>> + * Copyright (c) 1997, 2012, Oracle and/or its affiliates. All rights reserved.
>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>> *
>>>>> * This code is free software; you can redistribute it and/or modify it
>>>>> @@ -339,7 +339,7 @@
>>>>> return -1;
>>>>> }
>>>>> }
>>>>> - return buf[pos++] & 0xff;
>>>>> + return Byte.toUnsignedInt(buf[pos++]);
>>>>> }
>>>>>
>>>>> public int read(byte[] b, int off, int len) throws IOException {
>>>>> diff -r 303b67074666 src/share/classes/java/util/zip/InflaterInputStream.java
>>>>> --- a/src/share/classes/java/util/zip/InflaterInputStream.java Tue Jan 24 15:13:27 2012 -0500
>>>>> +++ b/src/share/classes/java/util/zip/InflaterInputStream.java Wed Jan 25 20:31:05 2012 -0800
>>>>> @@ -1,5 +1,5 @@
>>>>> /*
>>>>> - * Copyright (c) 1996, 2006, Oracle and/or its affiliates. All rights reserved.
>>>>> + * Copyright (c) 1996, 2012, Oracle and/or its affiliates. All rights reserved.
>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>> *
>>>>> * This code is free software; you can redistribute it and/or modify it
>>>>> @@ -119,7 +119,7 @@
>>>>> */
>>>>> public int read() throws IOException {
>>>>> ensureOpen();
>>>>> - return read(singleByteBuf, 0, 1) == -1 ? -1 : singleByteBuf[0] & 0xff;
>>>>> + return read(singleByteBuf, 0, 1) == -1 ? -1 : Byte.toUnsignedInt(singleByteBuf[0]);
>>>>> }
>>>>>
>>>>> /**
>>>>> diff -r 303b67074666 src/share/classes/java/util/zip/ZipInputStream.java
>>>>> --- a/src/share/classes/java/util/zip/ZipInputStream.java Tue Jan 24 15:13:27 2012 -0500
>>>>> +++ b/src/share/classes/java/util/zip/ZipInputStream.java Wed Jan 25 20:31:05 2012 -0800
>>>>> @@ -1,5 +1,5 @@
>>>>> /*
>>>>> - * Copyright (c) 1996, 2009, Oracle and/or its affiliates. All rights reserved.
>>>>> + * Copyright (c) 1996, 2012, Oracle and/or its affiliates. All rights reserved.
>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>> *
>>>>> * This code is free software; you can redistribute it and/or modify it
>>>>> @@ -435,7 +435,7 @@
>>>>> * The bytes are assumed to be in Intel (little-endian) byte order.
>>>>> */
>>>>> private static final int get16(byte b[], int off) {
>>>>> - return (b[off] & 0xff) | ((b[off+1] & 0xff) << 8);
>>>>> + return Byte.toUnsignedInt(b[off]) | (Byte.toUnsignedInt(b[off+1]) << 8);
>>>>> }
>>>>>
>>>>> /*
>>>>> diff -r 303b67074666 src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipConstants.java
>>>>> --- a/src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipConstants.java Tue Jan 24 15:13:27
>>>>> 2012 -0500
>>>>> +++ b/src/share/demo/nio/zipfs/src/com/sun/nio/zipfs/ZipConstants.java Wed Jan 25 20:31:05
>>>>> 2012 -0800
>>>>> @@ -185,11 +185,11 @@
>>>>> */
>>>>> ///////////////////////////////////////////////////////
>>>>> static final int CH(byte[] b, int n) {
>>>>> - return b[n] & 0xff;
>>>>> + return Byte.toUnsignedInt(b[n]);
>>>>> }
>>>>>
>>>>> static final int SH(byte[] b, int n) {
>>>>> - return (b[n] & 0xff) | ((b[n + 1] & 0xff) << 8);
>>>>> + return Byte.toUnsignedInt(b[n]) | (Byte.toUnsignedInt(b[n + 1]) << 8);
>>>>> }
>>>>>
>>>>> static final long LG(byte[] b, int n) {
>>>>>
>>>>> If the changes look good, I'll file a bug for them, etc.
>>>>>
>>>>> In case other people want to look over candidates sites in different areas, I've included the
>>>>> list of JDK files using "0xff" below.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Joe
>>>>>
>>>>> [1] 4504839: Java libraries should provide support for unsigned integer arithmetic
>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-January/008926.html
>>>>>
>>>>> .
>>>
>
>
More information about the core-libs-dev
mailing list