Old mmap->lazyio, add new mmap #79
Conversation
This PR reworks the handling of "mmapped" images, preserving the previous implementation and making it accessible with `lazyio=true`. The main motivation is to take advantage of the OS-provided buffering of mmapped data, so that elementwise access patterns become much more efficient, particularly for 3d images. (In some simple demos, by multiple orders of magnitude.) I went with a "name swap" because calling the previous implementation "mmap" is a misnomer: it is a useful form of lazy-loading, but there was no use of "true" mmapping in the sense of `Mmap.mmap`. This also introduces the ability to open a mmapped TIFF with `r+` permissions and thus write (simply by setting values) as well as read.
|
I was going to use Also, you'll note I did a minor version bump. There are enough behavioral changes:
that I was concerned that this is more than an "internal detail" and will require users to vet this more carefully. |
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
==========================================
+ Coverage 90.69% 91.04% +0.34%
==========================================
Files 12 13 +1
Lines 688 748 +60
==========================================
+ Hits 624 681 +57
- Misses 64 67 +3
Continue to review full report at Codecov.
|
|
Thanks @timholy. I think this is a reasonable set of changes. Less flexible, but substantially better worse case behavior. The original approach is a disaster if you do things like JuliaArrays/MappedArrays.jl#49, this PR basically solves that problem completely. Could you update the precompile statement to improve the time to first image? TiffImages.jl/src/TiffImages.jl Lines 39 to 46 in facdf3d |
Also fix a bug in sizing of the slice buffer for 1-byte eltypes
|
OK, I added precompilation (great suggestion!) and updated the docs. The doc update will fail, because the docs depend on ImageIO which depends on TiffImages. Consequently, any breaking release of TiffImages will fail its docbuild until a compatible version of ImageIO is released. If you can refactor it to make the docs independent of ImageIO, it would fix the problem. One possible strategy is to ditch Images.jl for ImageCore.jl; if you can do that ubiquitously then that alone might solve it. Also, when building the docs I got this warning: Not sure if it's important. |
|
Also, something worth considering: I suspect that long-term, we want One could call this effectively a deprecation warning, only bumping the minor version when |
Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>
|
Yeeeah~ 🎉 some new errors 😂 |
|
I have access to a Windows machine, let me try to fix it locally first. |
| seek(file, 0) | ||
| rawio = stream(file) | ||
| raw = Mmap.mmap(rawio, Vector{UInt8}, filesize(rawio)) | ||
| chunks = [getchunk(T, raw, reverse(sz), ifd) for ifd in ifds] | ||
| return MmappedTIFF{T, N, O, eltype(chunks)}(chunks, ifds, sz) |
There was a problem hiding this comment.
I have access to a Windows machine, let me try to fix it locally first.
I believe timholy#1 fixes the test. But it seems to uncover a windows caveat that mmap-ed images should be surrounded by a let block and eager GC.gc()
timholy#2 is an attempt to eagerly do GC.gc() if issues like this happens. But one should still do GC.gc() before trying to remove the mmapped files. But I'm not sure...
There was a problem hiding this comment.
MWE:
using ImageCore
using TiffImages
using ImageShow
img = rand(Gray, 64, 64)
TiffImages.save("tmp.tif", img)
img = TiffImages.load("tmp.tif"; mmap=true)
img2 = TiffImages.load("tmp.tif") # works
TiffImages.save("tmp.tif", img2) # fails -- because img occupies the file handlerWith timholy#2
img2 = let img = TiffImages.load("tmp.tif"; mmap=true)
TiffImages.load("tmp.tif") # works
end
TiffImages.save("tmp.tif", img2) # works -- with an warning┌ Warning: failed to open file tmp.tif in "w" mode, retry after GC.gc()
└ @ TiffImages d:\Julia\TiffImages.jl\src\utils.jl:205There seems to be no good way to work around it. All I can imagine is to clearly document
that mmapped images should be used in as small as possible local scope, otherwise, it
would easily lock the file and block or fail IO operations from other tasks.
There was a problem hiding this comment.
Now I believe that we should probably not abuse the load interface. But instead, we should use something similar to the open() context since mmap requests a file resource -- it should follow the C++ RAII (Resource Acquisition Is Initialization) practice.
TiffImages.lazy_load(filepath) do img
...
endcould even support multiple files version,
TiffImages.lazy_load(files::Tuple) do imgs
...
endThere was a problem hiding this comment.
I'd be fine introducing an mmap into FileIO. But for reference, NRRD.jl has been supporting open(filename; mmap=true) for a long time without issue. It's even the default for large images, you have to explicitly turn of mmap if you don't want it. Presumably, it has been real-world tested mostly (or exclusively?) on Linux.
Co-authored-by: 陈久宁 <chenjiuning@tongyuan.cc>
|
Let's wait for timholy#2 before merging this. |
| if mmap | ||
| ifd = first(ifds) | ||
| if mmap && iscontiguous(ifd) && getdata(CompressionType, ifd, COMPRESSION, COMPRESSION_NONE) === COMPRESSION_NONE | ||
| return MmappedTIFF(tf, ifds) |
There was a problem hiding this comment.
Let's wait for timholy#2 before merging this.
I now don't believe #2 is the correct fix. We should fix the semantics instead.
Note the close(tf.io) in R42 -- this means mmap=true never correctly close the file IO -- the fact that linux CI passes the test is just some bad luck.
Instead, we should use the explicit mmap semantic in timholy#3 and it's a perfect fix for https://github.com/tlnagy/TiffImages.jl/pull/79/files#r880478304
img = rand(Gray, 64, 64)
TiffImages.save("tmp.tif", img)
sz = TiffImages.mmap("tmp.tif") do img
size(img)
end
img2 = TiffImages.load("tmp.tif") # works
TiffImages.save("tmp.tif", img2) # worksBut I can't managed to directly write to the file using mode="w", maybe I'm using it incorrectly?
# it seems that every time when the file is opened, it get cleaned up..
TiffImages.mmap("tmp.tif", mode="w") do img
img .= img[1]
endWe could even bring this mmap function to FileIO, although I'm not sure if it is feasible.
There was a problem hiding this comment.
the fact that linux CI passes the test is just some bad luck.
Not really, see https://stackoverflow.com/questions/42961339/mmap-after-deleting-the-file. I wish Windows did this. I think that's really the root of all the problems on Windows, and everything works nicely as you'd hope on Linux.
|
I think these problems with Windows files occur mostly in tests. How often do you really expect this to happen in practice? If someone has gone to the effort to collect a 100GB image series, are they really likely to just delete it? |
| # These work only if the file was opened with write permissions | ||
| Base.@propagate_inbounds Base.setindex!(img::MmappedTIFF{T, 2}, val, i::Int, j::Int) where T = chunk1(img)[j, i] = val | ||
|
|
||
| Base.@propagate_inbounds function Base.setindex!(img::MmappedTIFF{T, 3}, val, i::Int, j::Int, k::Int) where T | ||
| chunk = img.chunks[k] | ||
| chunk[j, i] = val | ||
| end |
There was a problem hiding this comment.
julia> img = TiffImages.load("tmp.tif"; mmap=true)
julia> img[2] = img[1]
ERROR: ReadOnlyMemoryError() <--- this seems an enctrypted error message to the first time user
Stacktrace:
[1] setindex!
@ .\array.jl:903 [inlined]
[2] setindex!An idea here can be explicitly constructing the MmappedTIFF with a bool value so that we can eagerly detect and throw errors.
struct MmappedTIFF{T <: Colorant, N, Writable, O <: Unsigned, A <: AbstractMatrix{T}} <: AbstractTIFF{T, N}
...
end
...
Base.@propagate_inbounds Base.setindex!(img::MmappedTIFF{T, 2, true}, val, i::Int, j::Int) where T = chunk1(img)[j, i] = val
Base.@propagate_inbounds function Base.setindex!(img::MmappedTIFF{T, 3}, val, i::Int, j::Int, k::Int) where T
chunk = img.chunks[k]
chunk[j, i] = val
end
function Base.setindex!(img::MmappedTIFF{T, N, false}, val, i::Int, j::Int) where {T,N}
error("unable to write a readonly memory-mapped array, maybe you need to open the file with `r+` mode.")
endThere was a problem hiding this comment.
That's a good idea. Just to decrease need for compilation specialization (some may use both read-only and read-write in the same session), do you think we could do almost as well with register_error_hint? If so, that would require less duplication of compilation effort.
There was a problem hiding this comment.
The duplication of compilation is a valid reason to not do this.
But I'm not sure whether we can register an error hint for ReadOnlyMemoryError because it's an empty struct that we have no extra information
to examine and filter. Can register_error_hint get callstack information in general?
If not doable, the current best approach is just to mention ReadOnlyMemoryError error is due to file mode in the documentation.
examples/mmap_lazyio.jl
Outdated
| # !!! warning | ||
| # Setting values in the array writes those same values to disk! | ||
| # Careless use of `mode="r+"` can easily corrupt raw data files. | ||
| # You should omit `mode`, or use `mode="r"` (read-only), unless | ||
| # you intend to rewrite files. |
There was a problem hiding this comment.
I think these problems with Windows files occur mostly in tests. How often do you really expect this to happen in practice? If someone has gone to the effort to collect a 100GB image series, are they really likely to just delete it?
Although I believe the right fix is to not abuse the load interface, I get your point that we probably
don't need to bother with this edge case issue. Just that I'm not sure how broadly this affects people's
daily life usage.
But I would suggest a clear warning on this issue, especially for windows, and I believe we should also
explicitly document somewhere in the docstring that closing file will be delayed to Julia's GC when
the image object is not used anymore. -- not appropriately closing the file is what I'm worried.
|
This PR looks good from my end. I'm open to whatever you all think re: Windows. I won't merge until I get the all clear from @timholy. It's probably a good idea to withhold from updating |
Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>
There was a problem hiding this comment.
Very informative documentation! I guess we just need to merge timholy#2 before merging this PR?
|
Thanks!This was a very productive discussion and I think users will be better-served for it.
I think that makes sense. I left a suggestion for an alternative implementation, but we can go with yours if you prefer it.
We should also suppress "canonicalization" if either |
This attempts to head off a potential bug on Windows: if a file was previously mmapped in the same session and someone tries to overwrite the file, the presence of the existing map will trigger a system error. Once can circumvent this if the old map is garbage-collected. This works only if there are no "live" references to the memory, but it does not hurt to try. Perhaps even more importantly, if it fails then an informative warning is displayed. Co-authored-by: 陈久宁 <chenjiuning@tongyuan.cc>
|
Presuming it passes tests, I think this is now ready to merge. |
|
Given that we renamed mmap to lazyio, @timholy would you mind renaming the |
|
How is But I did the rename and went with |
|
It's not incorrect per se, but in light of the introduction of the |
|
Sounds good to me. I don't have merge privileges, so from my standpoint merge whenever you feel comfortable. |
|
Thanks @timholy 🎉 , this will be a welcome performance improvement! |
This PR reworks the handling of "mmapped" images, preserving the
previous implementation and making it accessible with
lazyio=true.The main motivation is to take advantage of the OS-provided buffering
of mmapped data, so that elementwise access patterns become much
more efficient, particularly for 3d images. (In some simple demos,
by multiple orders of magnitude.)
I went with a "name swap" because calling the previous implementation
"mmap" is a misnomer: it is a useful form of lazy-loading, but
there was no use of "true" mmapping in the sense of
Mmap.mmap.This also introduces the ability to open a mmapped TIFF with
r+permissions and thus write (simply by setting values) as well as read.
Here's a quick demo of the performance improvement:
In this particular case it's an improvement of more than 4 orders of magnitude. The reason is that by switching slice planes, the
lazyio=trueversion has to re-read the active slice each time.What this should enable is applications like https://imagej.net/plugins/bdv/ in which the slice visualization plane may be chosen arbitrarily, without forcing users to write their data in 3d chunks using HDF5 or similar formats.
Closes JuliaArrays/MappedArrays.jl#49