Skip to content

TiffSaver: Only setLength of output file once#3239

Merged
sbesson merged 1 commit intoome:developfrom
dgault:TiffSaver-avoid-setLength
Dec 17, 2018
Merged

TiffSaver: Only setLength of output file once#3239
sbesson merged 1 commit intoome:developfrom
dgault:TiffSaver-avoid-setLength

Conversation

@dgault
Copy link
Copy Markdown
Member

@dgault dgault commented Oct 2, 2018

This is in relation to trying to address performance issues, see https://trello.com/c/OimiHAQY/39-ome-common-profile-tiff-writing and ome/ome-common-java#15

As shown in ome/ome-common-java#15 the problem lies in the repeated calling of setLength on RandomAccessFile.

The issue originally thought to be a Windows issue looks like it may be related to Java versions with RandomAccessFile.setLength being much slower on Java 10 (see https://stackoverflow.com/questions/50450317/randomaccessfile-setlength-much-slower-on-java-10-centos for some info)

In Java 8 setLength does:

[pid 49027] ftruncate(23, 53248)        = 0
[pid 49027] lseek(23, 0, SEEK_SET)      = 0
[pid 49027] lseek(23, 0, SEEK_CUR)      = 0

While in Java 10 it introduces the much slower fallocate (to fix a bug):

[pid   444] fstat(8, {st_mode=S_IFREG|0664, st_size=126976, ...}) = 0
[pid   444] fallocate(8, 0, 0, 131072)  = 0
[pid   444] lseek(8, 0, SEEK_SET)       = 0
[pid   444] lseek(8, 0, SEEK_CUR)       = 0

This is particularly problematic for us as the profiling shows we make a large number of calls to setLength. Essentially every time we write a strip we are calling it to increase the length of the output file.

Buffering in NIOFileHandle will help improve this, but as TiffSaver knows the length of data it is writing it might make more sense to make a single call to allocate the length upfront. In this case it is being forced by seeking to length which in turn calls setLength.

// sets the output file size without having to allocate for each strip iteration
out.seek(stripStartPos + totalStripSize);
// return to original position
out.seek(stripStartPos);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seek is unnecessary and can be removed

@dgault
Copy link
Copy Markdown
Member Author

dgault commented Oct 8, 2018

Attached show the before and after of the call stacks:
profile_before

After:
profile_after

Using bfconvert with tubhiswt-4D dataset:

Windows - Java 8
Without PR: 71.031s elapsed (4.6918607+74.8314ms per plane, 2125ms overhead)
With PR: 26.018s elapsed (4.816279+23.474419ms per plane, 1016ms overhead)

Windows - Java 10 (actually not impacted as much as expected)
Without PR: 78.485s
With PR: 23.783s

Tiling performance is not improved so that needs to be corrected.

@dgault
Copy link
Copy Markdown
Member Author

dgault commented Oct 9, 2018

As a follow up, i've been profiling and debugging the tiled scenario further. In the case of tiling there are some differences but the short of it is we are still in a scenario of having to setLength for each tile so performance will remain similar as to before. In saying that the setLength doesnt impact the tiled case as much in the first place, the reading of tiles in ImageConverter is actually adding overhead as opposed to the writing.

@sbesson sbesson self-requested a review December 17, 2018 12:36
Copy link
Copy Markdown
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change make sense and this has certainly been tested over the last weeks for various conversions without any adverse effect being reported. All unit tests are passing. Merging for the next milestone of Bio-Formats 6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants