Re: RFR: JDK-8150496,(zipfs) Fix performance issues in zip-fs

[Xueming Shen]

Hi Aleksey,

Again, thanks for the review. The webrev has been updated accordingly,
as well as the MyBenchmark.java (to use Blackhole, as suggested)

http://cr.openjdk.java.net/~sherman/8150496/webrev/
http://cr.openjdk.java.net/~sherman/8150496/MyBenchmark.java

-Sherman


On 05/03/2016 05:39 AM, Aleksey Shipilev wrote:

Hi Sherman,

On 05/03/2016 02:28 AM, Xueming Shen wrote:

Please help review the performance cleanup for zipfs

issue: https://bugs.openjdk.java.net/browse/JDK-8150496
webrev: http://cr.openjdk.java.net/~sherman/8150496/webrev

ZipFileSystem.java:

*) These should be removed?

  2032             //writeShort(os, name.length);
  2033             writeShort(os, nlen);

  2220             //writeShort(os, name.length);
  2221             writeShort(os, nlen);

  2591         //inodes.forEach( (k,v) ->  { System.out.printf(" %s  ->
%s%n", new String(k.name), new String(v.name)); });

  1080             /*
  1081             byte[] name = Arrays.copyOfRange(cen, pos + CENHDR,
pos + CENHDR + nlen);
  1082             */

*) There is ever-so-subtle difference in doing either:

  171             byte[] tmp = new byte[path.length + 1];
  172             System.arraycopy(path, 0, tmp, 1, path.length);
  173             tmp[0] = '/';

...or:

1083             byte[] name = new byte[nlen + 1];
1084             name[0] = '/';
1085             System.arraycopy(cen, pos + CENHDR, name, 1, nlen);

Excess op between allocation and arraycopy breaks zeroing elimination,
see e.g.:
   http://cr.openjdk.java.net/~shade/scratch/ZeroingFirstBench.java


*) Missing braces:

1483                 if (e2 == null)
1484                     throw new ZipException("invalid loc for entry
<" + e.name +">");

*) Probably deserves a comment why these are commented out? Are these
implied to be zeroes?

1713         // int  disknum;
1714         // int  sdisknum;

Here are the result of the sample benchmark that measures the improvement.
http://cr.openjdk.java.net/~sherman/8150496/MyBenchmark.java

This benchmark is not very reliable:

  *) Sink everything in Blackhole. I.e. instead of, say:

     @Benchmark
     public void ZFS_Open() throws IOException {
         cnt = 0;
         for (int i = 0; i<  100; i++) {
             try (FileSystem fs = newFileSystem(path)) {
                 if (Files.exists(fs.getPath(entriesS.get(0))))
                    cnt++;
             }
         }
     }

do:

     @Benchmark
     public void ZFS_Open(Blackhole bh) throws IOException {
         for (int i = 0; i<  100; i++) {
             try (FileSystem fs = newFileSystem(path)) {
                 bh.consume(fs.getPath(entriesS.get(0)));
             }
         }
     }

This applies to all other methods.

Thanks,
-Aleksey