Reduce calls to RandomAccessFile.setLength in NIOFileHandle

[melissalinkert]

Instead of calling raf.setLength with the exact length required for the current write, this increases the length by NIOFileHandle.getBufferSize() in anticipation of future writes. NIOFileHandle.close() then reconciles any discrepancy between the actual and expected file lengths, so that there are no extra bytes in the file. This was the least invasive change I could think of, but happy to discuss if anyone has other thoughts.

See https://trac.openmicroscopy.org/ome/ticket/6931 and https://trello.com/c/OimiHAQY/175-profile-tiff-writing-on-windows. This does have a noticeable impact on the performance of bfconvert on Windows; in the test case on the ticket, local Windows times are now down to ~52s vs ~95s.

[sbesson]

Sorry for the delay @melissalinkert, slowly getting back to this. Is there a minimal example consuming the loci.common.NIOFileHandle API that could be run on Linux and Windows with and without this PR to test its effect in isolation of Bio-Formats?

[melissalinkert]

I'd expect that something as simple as:

NIOFileHandle out = new NIOFileHandle("test", "rw");
long byteCount = 1024 * 1024; // 1 MB
for (long i=0; i<byteCount; i++) {
  out.write(1);
}
out.close();

would be noticeably faster with this PR.

[dgault]

The code looks good and the solution is effective without being a major overhaul.

Testing using the sample code suggested provides a substantial improvement in performance.

Without the PR:

1024 * 1024: Time elapsed: 11898 ms
2048 * 2048: Time elapsed: 36811 ms
4096 * 4096: Time elapsed: 147633 ms

With the PR:

1024 * 1024: Time elapsed: 2089 ms
2048 * 2048: Time elapsed: 7692 ms
4096 * 4096: Time elapsed: 31783 ms

[sbesson]

Looks good and should be ready to merge. In terms of integration:

• one question about the impact of propagation of this feature up the stack
• more generally, in order to cut a new ome-common release, we should agree on whether the changes made in Location NPE fix #12 should be supplemented with some backwards compatible API - see https://trello.com/c/Ym1HWlQG/159-ome-common-location-exception /cc @dgault

[sbesson]

Am I correct in assuming that the addition of a private field means this would break serialization when propagated up to the Bio-Formats stack

[melissalinkert]

Not necessarily - readers shouldn't be caching open file handles, so I wouldn't think instances of NIOFileHandle would be present in memo files. For example:

https://github.com/openmicroscopy/bioformats/blob/develop/components/formats-api/src/loci/formats/FormatReader.java#L127

Probably merits a test before bumping the ome-common version number in Bio-Formats though, to be safe.

[sbesson]

Using the following Makefile to build Bio-Formats with and without this PR:

BUILDDIR=/tmp/build

clean:
	rm -rf $(BUILDDIR)
	mkdir -p $(BUILDDIR)
	test -f bioformats-5.7.0.zip || wget https://downloads.openmicroscopy.org/bio-formats/5.7.0/artifacts/bioformats-5.7.0.zip
	test -f setlength-performance.zip || wget https://github.com/melissalinkert/ome-common-java/archive/setlength-performance.zip

bf1:
	unzip bioformats-5.7.0.zip && mv bioformats-5.7.0 $(BUILDDIR)/bf1
	ant tools -f $(BUILDDIR)/bf1/build.xml -Dmaven.repo.local=$(BUILDDIR)/.m2/repository

ome-common:
	unzip setlength-performance.zip && mv ome-common-java-setlength-performance $(BUILDDIR)/ome-common
	mvn clean install -f $(BUILDDIR)/ome-common/pom.xml -Dmaven.repo.local=$(BUILDDIR)/.m2/repository

bf2:
	unzip bioformats-5.7.0.zip && mv bioformats-5.7.0 $(BUILDDIR)/bf2
	sed -i '' -e 's/5.3.2/5.3.3-SNAPSHOT/g' $(BUILDDIR)/bf2/pom.xml
	ant tools -f $(BUILDDIR)/bf2/build.xml -Dmaven.repo.local=$(BUILDDIR)/.m2/repository -Dome-common.version=5.3.3-SNAPSHOT
     
.PHONY: clean bf1 ome-common bf2

and running /tmp/build/bf{1,2}/tools/showinf -nopix -cache on a few filesets, I did not see any invalidated cache files.

From an integration perspective, this makes this PR eligible for an inclusion in a patch release of Bio-Formats. The main issue is the ongoing discussion on #12 (see https://trello.com/c/Ym1HWlQG/159-ome-common-location-exception) and the fact more discussion/configuration might be required before propagating the latter changes.

In order to move forward, my proposal would be to revert #12, merge this into master and cut a patch release ome-common:5.3.3 with this performance improvement. We can then propagate the release down the stack up to Bio-Formats. Would that work for you @melissalinkert @dgault ?