Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up HDF5 processing #110

Merged
merged 24 commits into from
Aug 24, 2023
Merged

Speed up HDF5 processing #110

merged 24 commits into from
Aug 24, 2023

Conversation

emilmelnikov
Copy link
Member

@emilmelnikov emilmelnikov commented Jul 28, 2023

Previously, HDF5 datasets were always read and written plane-wise (XY). Also, pixel values were always boxed when reading. This resulted in sub-optimal performance, especially when the source HDF5 did not have XY as it's fastest changing dimensions.

The new reader implementation always reads data either in native HDF5 chunks, or in the largest possible contiguous blocks, and pixel values are never boxed. Moreover, if the source image is not chunked and sufficiently small (element count is less than 2^31), a simpler ArrayImg (just a simple wrapper around a primitive array) is used instead of CellImg (ND grid of ArrayImg).

The writer still deals with boxed pixels: we need to be able to write (almost) arbitrary ImgPlus which could contain anything. However, it tries to minimize overhead by creating flat primitive arrays from large blocks obtained from the source ImgPlus and write them directly. Note that these blocks are large and different from HDF5 chunks which are usually much smaller.

Another important change is using image views both when reading and writing instead of trying to actually change data dimensions. When reading, source dimension order is preserved, but before returning it is (virtually) permuted to match the requested axis order. When writing, data is (also virtually) permuted before obtaining blocks and performing actual writes.

Additionally, support for other primitive data types has been added.

Also fix some bugs and adapt tests.
Copy link
Contributor

@k-dominik k-dominik left a comment

Choose a reason for hiding this comment

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

Hey @emilmelnikov,

when I first loaded a file with this code I couldn't believe it! Before it was 2 minutes, and with your changes it feels almost instant. That's really awesome!!

Also thank you for running me through the changes in person, really nice work. I left some comments of what we discovered during the walkthrough.

This partially replaces logService from the old implementation.
* Use no-op callbacks if none was provided
* Add error checking
* if axes are specified, image will be written in the specified axis order.
* <p>
* <p>
* If not null, callback will be invoked each time a block is about to be written.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If not null, callback will be invoked each time a block is about to be written.
* If not null, callback will be invoked each time a block is about to be written. This is used to provide feedback via progress bar.

Copy link
Contributor

@wolny wolny left a comment

Choose a reason for hiding this comment

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

fantastic job! Code is not only more efficient, but much more elegant (using streams and optionals). Can't wait to see it released!

HDF5 chunk sizes are usually quite small. At the same time, we want cells in `CellImg` to be large (larger cells means less overhead).
@emilmelnikov
Copy link
Member Author

emilmelnikov commented Aug 23, 2023

I've made some more changes.

Now HDF5 chunk size and imglib2 cell size are always separate. When reading chunked HDF5 file, it's chunk size no longer used as a cell size because chunk size is too small. When writing, cell size is chosen to be at most 256 MiB, which should be a good compromise between interactivity and performance. Write cell shape is now always either XY or XYZ with singleton dimensions for other axes.

When writing, callbacks now trigger status bar updates, so users can see the progress. Callback signature is changed: now it accepts long which is the total number of bytes written (it is easier to deal with bytes because we can precalculate the total number of bytes we need to read/write in advance, and use this for progress bar updates).

I think now this is really ready to be merged, if everything else is OK.

@emilmelnikov
Copy link
Member Author

Side note: imglib2 recently added PrimitiveBlocks for copying an arbitrary subregion into a raw array. I've benchmarked it for writing a "simple" dataset, and it is faster by 20–40% depending on the specific axis order! However, if the target dataset is a more complex view, it switches to fallback implementation which, in my tests, resulted in out-of-heap error. Therefore, I left existing implementation for now.

Copy link
Contributor

@k-dominik k-dominik left a comment

Choose a reason for hiding this comment

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

Really nice! With all the eyes this PR got and all the praise, too, let's not delay merging it (pls don't squash).

The only minor cosmetic issue I noticed is that the progress bar is not reset to zero when writing during workflow processing. It's reset after processing is done though, so, also fine to merge without fixing it.

emilmelnikov and others added 2 commits August 24, 2023 14:54
This script could be used to test HDF5 reader
If not cleared, progress in the status bar do not disappear.

Co-authored-by: Dominik Kutra <k-dominik@users.noreply.github.com>
@emilmelnikov emilmelnikov merged commit 08c603e into master Aug 24, 2023
1 check passed
@emilmelnikov emilmelnikov deleted the faster-hdf5 branch August 24, 2023 13:00
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/ilastik-instance-handling-in-fiji-macros/86286/2

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.

None yet

5 participants