ome / ome-codecs

OME image compression (coder-decoder implementations)
Other
4 stars 13 forks source link

ZLIB: use ByteArrayHandle instead of ByteVector for decompressing #11

Closed melissalinkert closed 1 year ago

melissalinkert commented 6 years ago

Partial backport from a private PR.

ByteVector doesn't have any functionality not present in ByteArrayHandle. This commit lets us to measure the impact of switching to ByteArrayHandle in an isolated case, which would allow for an informed decision about whether it makes sense to deprecate ByteVector.

With a *.C01 file from data_repo/curated/cellomics/ (e.g. anything in the omar subdirectory) and test code similar to:

import loci.common.RandomAccessInputStream;
import ome.codecs.ZlibCodec;

public class Test {
  public static void main(String[] args) throws Exception {
    String file = args[0];
    int count = Integer.parseInt(args[1]);
    ZlibCodec codec = new ZlibCodec();
    long decompressTime = 0;
    for (int i=0; i<count; i++) {
      RandomAccessInputStream s = new RandomAccessInputStream(file);
      s.seek(4);
      long t0 = System.currentTimeMillis();
      codec.decompress(s, null);
      long t1 = System.currentTimeMillis();
      s.close();
      decompressTime += (t1 - t0);
    }
    /* debug */ System.out.println("decompress time: " + decompressTime + "ms (avg: " + ((double) decompressTime / count) + "ms)");
  }
}

there shouldn't be a statistically significant difference in performance with or without this PR. If performance is noticeably worse or there are other reasons to continue using ByteVector, this can be closed outright.

melissalinkert commented 6 years ago

Closing temporarily for https://web-proxy.openmicroscopy.org/east-ci/job/BIOFORMATS-merge/148/jdk=JDK8,label=testintegration/console

melissalinkert commented 6 years ago

8988246 adds a simple roundtrip compression/decompression sanity check for ZLIB which will fail with 0b3616d alone. 3e488d7 fixes the test failure, so I'd hope that this now passes the build. Expanding the roundtrip test to cover more codecs or edge cases might be a good idea for the future (not sure if we can migrate anything from bioformats).

sbesson commented 6 years ago

The changes here seem to affect the consumption of ZlibCodec by NativeND2Reader under certain conditions - see https://web-proxy.openmicroscopy.org/east-ci/job/BIOFORMATS-test-folder/29043/console. Excluding for now to reduce the number of failures, feel free to re-include in the builds once a fix is pushed.

melissalinkert commented 6 years ago

f3aad26 should fix the tests, but re-including isn't urgent so probably makes sense to wait until things are a little quieter.

sbesson commented 1 year ago

As discussed during the weekly Formats meeting, given this PR does not substantially improve performance, closing for now as no additional work is planned immediately.