Re: Using unsigned library work in the JDK - please rename them!

Tue, 07 Feb 2012 07:25:41 -0800 

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

.

Reply via email to