RFC: mmap makeover

[quinnj]

[Updated to reflect latest changes]

This PR overhauls the overall mmap interface by the following changes:

• Removes the low-level Libc.mmap interface: it was only implemented for unix, and didn't even match the full mmap functionality that's available (i.e. anonymous maps). The implementation was weirdly split between mmap_array and mmap, except for windows, where the entire implementation was in mmap_array. The core mmap code is now completely consolidated within the core Mmap.mmap function, which represents the real, unifying interface that is cross-platform.
• Replaces mmap_array with Mmap.mmap which returns a regular Array whose contents point to a memory-mapped region. Several convenience constructors are added for working on file names directly.
• Mmap.mmap takes an ::Type{Array{T,N}} as input determining the resulting Array type to return; this is to provide consistency between Mmap.mmap(io, Vector{UInt8}...) and Mmap.mmap(io, BitVector...)
• Adds full support for "anonymous" mmaps, which are not backed by files. On Windows, this was implemented by the SharedMemSpec type, and this didn't exist for unix. Now, they are unified through the Mmap.Anonymous IO type, more generally useable through Mmap.mmap{T}(::Type{Array{T,N}}, dims)).
• Fixes segfaults where a negative offset was specified to mmap_array
• Consolidates and adds additional argument checks for Mmap.mmap (e.g. check that io is open, that we have a sane len argument, that a file has write permissions if we need to grow it, that Array{T,N} is a bits type, etc.)
• Removes the munmap method in favor of automatically finalizing an mmapped-Array upon construction; this removes the possibility of a user manually munmapping and then getting a segfault when trying to access a memory-mmaped array
• Replaces msync with Mmap.sync! to flush changes made to a mmapped-Array back to disk

+574/ -299 LOC diff mainly consists of:

• +117 deprecated methods
• 4x the number of mmap tests (~70 before, ~284 now)
  The core implementation is actually smaller than before (~175 lines vs. ~235 lines), even with additional functionality.

[timholy]

👍 My main question is just whether Stream is the appropriate term here, since to me that implies something to which you may not have random access.

[timholy]

This will cause trouble for anyone who is actually wanting to mmap an array of bools, i.e., UInt8 with values of 0 or 1.

[timholy]

I also wonder if we should add a convenience function mmap_array(T, dims, filename, mode). One thing users often wonder is whether it's OK to close the IOStream but still work with the array; I know that's what you should do on unix. I'm having a harder time finding the answer for Windows. If you need to keep the IOStream open on windows, then we definitely can't add that convenience function.

[quinnj]

Yes, good point on the Stream perhaps not implying random access, though on the other hand, it is a very IO-y kind of thing, so in that sense, Stream makes sense (which is the angle I was coming from). I wanted to provide some functionality for getting at the raw memory chunk without necessarily allocating an entire Julia array first (I'm looking to do some file processing where you want to first mmap it, poke around a bit, then pull things out selectively).

Yes, I also had the idea of adding the convenience functions. From what I understand, we should be clear on Windows, since it uses a separate OpenFileMapping process entirely (and also has the SharedMemSpec path that doesn't even deal with open IOStream).

[quinnj]

Anyone know what's up with the map! segfaults on travis? Is that unrelated I'm hoping?

[quinnj]

@twadleigh, @amitmurthy, @carlobaldassi. You three seem to be the local sharedarray.jl experts. Any idea why the changes here are causing SharedArray to segfault on unix? (it seems that any call to getindex on the SharedArray is causing the segfault).

I tried to not touch the actual implementation details of mmap at all and just copy/paste code around, but in my initial debugging efforts, I haven't uncovered what the cause here is.

[twadleigh]

When I implemented the Windows version of SharedArray I went through some uncomfortable contortions to keep the line change count down to a minimum. I did that in the interest of making the PR as palatable as possible. One of the things I liked least about the resulting design was the icky Union type in the signature of mmap_array. I see that it has survived/prevailed in your RFC. 😧

I think defining a consistent mmap interface across platforms is (1) a worthy goal, and (2) overdue. Even better would be if the resulting interface admitted not-entirely-unnatural implementations on each platform.

I don't have any easy answers for you right now, but I'll devote some time to pondering this issue this weekend.

[tkelman]

I went through some uncomfortable contortions to keep the line change count down to a minimum. I did that in the interest of making the PR as palatable as possible.

In retrospect I was too conservative in my reviews on those PR's, we were much earlier in the 0.3.x series at the time and making sure things could be made safe to backport was a big concern for me.

If we can get this settled in the next few weeks (total guess on timeline btw, don't take anything I say too seriously) before soft-freezing 0.4-pre, it would be good to brainstorm a better, more elegant API across all platforms. I'm not too familiar with the low-level subtleties here, but you have my moral support for being a bit aggressive with this.

[timholy]

Does A = SharedArray(Float32, (5,5)); A[1,2] crash for you when running a single process? If so, it will be much easier to debug.

[quinnj]

It does indeed @timholy;

julia> A = SharedArray(Float32, (5,5))
right before calling init function
Float32[
signal (11): Segmentation fault: 11
anonymous at sharedarray.jl:219
jl_apply at /Users/jacobquinn/julia/src/gf.c:1750
jl_apply at /Users/jacobquinn/julia/src/./julia.h:1290
isassigned at abstractarray.jl:89
print_matrix_repr at show.jl:1158
showarray at show.jl:1196
jlcall___showarray#22___113606 at  (unknown line)
jl_apply at /Users/jacobquinn/julia/src/gf.c:1750
julia_showarray_113604 at  (unknown line)
showall at show.jl:1229
jl_apply at /Users/jacobquinn/julia/src/gf.c:1750
SharedArray at sharedarray.jl:97
julia_call_113513 at  (unknown line)
jlcall_call_113513 at  (unknown line)
jl_apply at /Users/jacobquinn/julia/src/gf.c:1750
jl_apply at /Users/jacobquinn/julia/src/interpreter.c:55
eval at /Users/jacobquinn/julia/src/interpreter.c:212
eval at /Users/jacobquinn/julia/src/interpreter.c:218
eval_body at /Users/jacobquinn/julia/src/interpreter.c:579
jl_toplevel_eval_body at /Users/jacobquinn/julia/src/interpreter.c:514
jl_toplevel_eval_flex at /Users/jacobquinn/julia/src/toplevel.c:464
jl_eh_restore_state at /Users/jacobquinn/julia/src/./julia.h:1378
eval_user_input at REPL.jl:62
jlcall_eval_user_input_113505 at  (unknown line)
jl_apply at /Users/jacobquinn/julia/src/gf.c:1750
anonymous at task.jl:91
jl_apply at /Users/jacobquinn/julia/src/task.c:234
Segmentation fault: 11
jq-mbp:julia jacobquinn$

Can you give me a tip or two on debugging this? I'm happy to dive in, I just feel like a little direction would probably go along way. Thanks!

[timholy]

First do simple stuff like println(pointer(A.s)) to check for some obvious problem (like a NULL pointer), and possibly copy/pasting the contents of sharedarray.jl into a module MySharedArray ... end and inserting @show statements at relevant points to watch what happens during creation. If that doesn't suffice, do make debug and then gdb julia and try to capture the backtrace. More detail in http://docs.julialang.org/en/latest/devdocs/backtraces/ and http://docs.julialang.org/en/latest/devdocs/debuggingtips/

[twadleigh]

FYI, @quinnj, I'm starting to take a swing right now at a possible refactoring of mmap.jl and sharedarray.jl together. I'll point you at it when I have something to show.

[quinnj]

Ok, I think I've managed to tame the mmap beast here. The last few commits are another big iteration in merging all the design and I think it's really come together. Usage is now:

• Mmap.Array(::Type{T}, dims, io::IO, offset), which mirrors the old mmap_array
• Mmap.BitArray(... same as above, mirrors mmap_bitarray
• Mmap.Array(::Type{T}, dims, file::AbstractString, offset) for working directly on a file name
• Mmap.Array(io::Filename_or_IO, len, offset) for returning a byte array automatically
• Mmap.Stream(io::IO, len, offset) for mmapping an IO and returning a Stream object
• Mmap.Stream(file::AbstractString, len, offset) same as above, but with a filename
• read(::Mmap.Stream, UInt8) read a byte from an Mmap.Stream
• Mmap.sync!(::Mmap.Stream) for syncing an Mmap.Stream to disk
• Mmap.sync!(::Array_or_BitArray) for syncing Arrays/BitArrays that came from mmapped files to disk
• close(::Mmap.Stream) to close and unmap the file

There is now a single method definition for all platforms for each of the above functions (which means callees don't have to differentiate anymore), and that actually ended up consolidating a lot of code. Minus the 107 lines of additional tests, this is a net decrease of ~20 LOC for implementation. Nifty!

I think I've got the deprecations correct, but I should probably stress test them a little more. I also need to do a doc update, but otherwise, I'd appreciate further feedback on the design/overhaul.

(oh, and the segfault I was seeing earlier has been resolved too. All tests now pass for me locally on both windows 64-bit and OSX Yosemite 64-bit).

[tkelman]

There was a 32 bit Linux Travis failure on the push build but not the PR build, but it was just a spurious connection problem downloading Rmath-julia so I restarted it.

[carlobaldassi]

I only skimmed through the changes but from the summary I like this.

(As an aside: I wouldn't call myself a sharedarray expert in any way, I made some very limited contributions to those.)

[timholy]

If someone on 32bit wants access to the range 10-12GB in a large file, does making this an Int mean they are out of luck? Make it a FileOffset instead?

[ScottPJones]

Yes... as somebody who did a lot of accessing files in the TB range, on 32-bit machines, that really does need to be something bigger than a 32-bit int!

[timholy]

Aside from one small but important point, see nothing but goodness here. Nice work!

[quinnj]

@carlobaldassi, I'm doing another pass over the deprecations and remembered that I took out the following:

mmap_bitarray{N}(::Type{Bool}, dims::NTuple{N,Integer}, s::IOStream, offset::FileOffset) =
    mmap_bitarray(dims, s, offset)
mmap_bitarray{N}(::Type{Bool}, dims::NTuple{N,Integer}, s::IOStream) = mmap_bitarray(dims, s, position(s))

Any reason to keep something similar for the new Mmap.BitArray? I couldn't figure out why would we need those.

[carlobaldassi]

@quinnj: those definitions were there only for consistency with the mmap signature I think. They're quite useless indeed.

[ScottPJones]

👍 💯 This (and the other segment violations you've been finding) I think will be very useful (and much safer!)

[quinnj]

Hmmmm.......I feel like the Mmap.Array interface is really coming together (with guards against several previous ways to segfault), but I'm still running into an intermittent segfault when I run the test/mmap.jl file repeatedly (typically takes 5-13 runs of the test/mmap.jl to cause a segfault).

The output of running gdb is below, but the several different backtraces aren't really speaking to me. I'm not sure if this is a system thing or what necessarily; if anyone feels really kind and would help take a look, I'd really appreciate it. Even just by shedding some light (if any) on the gdb backtraces. Thanks.

https://gist.github.com/quinnj/599ba66ea141ae91cbc4

[quinnj]

I think I've made some progress? I've boiled it down to an interesting case. A stand-alone module you can run is here, and the script to produce the segfault is here. What's interesting is that running under gdb without the println reliably segfaults, whereas uncommenting the println makes the segfault go away. Makes me wonder what's really going on here?

It's fun getting into the gdb world of things, but honestly it's not my forte. Anyone feel up for helping debug this?

[quinnj]

One more update (sorry to be so noisy on this). Turns out it doesn't segfault in gdb with the println, but it still does at the REPL. At least at the REPL, I'm finally getting a consistent backtrace,

signal (11): Segmentation fault: 11
arraylist_pop at /Users/jacobquinn/julia/src/support/arraylist.c:72
run_finalizers at /Users/jacobquinn/julia/src/gc.c:129
jl_gc_collect at /Users/jacobquinn/julia/src/gc.c:2349
__pool_alloc at /Users/jacobquinn/julia/src/gc.c:1036
pool_alloc at /Users/jacobquinn/julia/src/gc.c:1086
allocobj at /Users/jacobquinn/julia/src/gc.c:2427
_new_array_ at /Users/jacobquinn/julia/src/array.c:84
_new_array at /Users/jacobquinn/julia/src/array.c:139
jl_alloc_array_1d at /Users/jacobquinn/julia/src/array.c:322
unlock at ./lock.jl:34
println at stream.jl:230
jl_apply at /Users/jacobquinn/julia/src/./julia.h:1290
jl_apply_generic at /Users/jacobquinn/julia/src/gf.c:1743
seg at none:10
jlcall_seg_44559 at  (unknown line)
jl_apply at /Users/jacobquinn/julia/src/./julia.h:1290
jl_trampoline at /Users/jacobquinn/julia/src/builtins.c:962
jl_apply at /Users/jacobquinn/julia/src/./julia.h:1290
jl_apply_generic at /Users/jacobquinn/julia/src/gf.c:1767
jl_apply at /Users/jacobquinn/julia/src/./julia.h:1290
do_call at /Users/jacobquinn/julia/src/interpreter.c:65
eval at /Users/jacobquinn/julia/src/interpreter.c:212
eval at /Users/jacobquinn/julia/src/interpreter.c:218
eval_body at /Users/jacobquinn/julia/src/interpreter.c:572
jl_interpret_toplevel_thunk_with at /Users/jacobquinn/julia/src/interpreter.c:610
jl_interpret_toplevel_thunk at /Users/jacobquinn/julia/src/interpreter.c:617
jl_toplevel_eval_flex at /Users/jacobquinn/julia/src/toplevel.c:522
jl_toplevel_eval at /Users/jacobquinn/julia/src/toplevel.c:530
jl_toplevel_eval_in at /Users/jacobquinn/julia/src/builtins.c:539
jl_f_top_eval at /Users/jacobquinn/julia/src/builtins.c:568
eval_user_input at REPL.jl:62
jlcall_eval_user_input_44514 at  (unknown line)
jl_apply at /Users/jacobquinn/julia/src/./julia.h:1290
jl_apply_generic at /Users/jacobquinn/julia/src/gf.c:1743
anonymous at task.jl:91
jl_apply at /Users/jacobquinn/julia/src/./julia.h:1290
jl_trampoline at /Users/jacobquinn/julia/src/builtins.c:962
jl_apply at /Users/jacobquinn/julia/src/./julia.h:1290
start_task at /Users/jacobquinn/julia/src/task.c:233
Segmentation fault: 11

I'm not sure what arraylist_pop does, but I'm going to go look!

[timholy]

You're calling close manually and then the finalizer is calling close again on the same object when it gets garbage collected. If you replace your test script's call to close with finalize, it doesn't segfault. (It also runs a lot more slowly!)

If you want to make it safe to call close directly, you can follow the HDF5/JLD model (which was implemented before finalize existed) and include a toclose::Bool field that you check before calling munmap. close would set toclose=false for that object.

Finally, I take back what I said about not wrapping it, of course you're right that just delegating all indexing operations to an internal array is fine and an easy way to get full functionality.

[quinnj]

Interesting. So is that a bug @timholy that I'm seeing a segfault in this case? I mean, I can manually create an Mmap.Array, close it, then run gc(), but don't see a segfault; so it seems that there's some kind of logic that knows that it's already been closed.

In any case, I'm already including isreadable and iswritable fields in the type to indicate permissions, so I just tweaked the close function to no-opt if both have already been set to false (which is what the no-opt version does). Segfault solved!

[timholy]

Not sure. If you run gc after each close, then perhaps you avoid a segfault because you don't have multiple finalizers trying to close overlapping blocks of memory? Anyway, glad you solved the segfault!

[simonster]

So, one problem is that if you access an mmapped array in a loop that modifies memory, you will probably have to load both isreadable/iswriteable and the array pointer each time. I'm not sure what the performance cost of that is, but it's worth benchmarking. The only way around that I can see is to make Mmap.Array immutable and not allow manual closing.

I think it would be good to leave the Libc functions. They aren't really any more obsolete now than they were when we had mmap_array.

[quinnj]

@simonster, you could access the underlying array Mmap.Array().array; though obviously that defeats some of the protection purpose, but it might be ok for hot loops. The idea of making it immutable is worth considering. Would there be any real drawbacks?

I just pushed another big iteration that solidifies the array interface for the new Mmap.Array type, as well as adds a lot more tests. I feel like this is shaping up well, so I'd appreciate any other comments on overall design. One note is that SharedArray now has an Mmap.Array field instead of a regular Array field. All the parallel.jl tests pass, but I'm not sure if this has broader implications for SharedArray. ([ci skip] because I still need to test this latest iteration on windows tonight)

[tkelman]

(though it's a pity it doesn't save the CI history somehow; i.e. I had 3 commits and all CI ran successfully on the last commit, I squashed the 3 together and force pushed with [ci skip]).

You can still find the past build results via navigating on Travis and AppVeyor. I wouldn't have put [ci skip] on the squashed final commit personally, unless the CI queue was a lot worse than it is right now.

[quinnj]

@carlobaldassi, here's a question I think I already know the answer to: when we do Mmap.mmap and want a BitArray, and use the default dims/len argument (that we're providing for arrays as well), we should assume the file is in binary format, right? I.e.

file = tempname()
open(file,"w") do f
    write(f,UInt64(1))
    write(f,UInt8(1))
end
m = Mmap.mmap(file, BitArray)

Should return a 72-element BitArray instead of 9-element, right? I guess we'd want to be consistent with how we write BitArrays out to a file; do we just write the bit values out?

[carlobaldassi]

Discussion here about the BitArray write format.

[quinnj]

@tkelman, any idea on the AppVeyor failures here? Things looked fine for me locally.

[tkelman]

sorry, screwed some things up by pushing straight to master like I keep telling other people not to do

[quinnj]

Alright, this is passing pretty much everywhere (the 64-bit AppVeyor timed out, but did run the file + mmap tests before timing out).

I've updated the top post to reflect the current API/changes. The main update here is the use of Array{T,N} as an input argument instead of just T to provide better consistency with BitArray{N}.

I'd appreciate any comments and feedback.

[quinnj]

Bump. Any thoughts? If not, I'll merge in a day or two.

[quinnj]

CI all passed; merging.

[ScottPJones]

👏 👍 Very nice!

[IainNZ]

According to http://pkg.julialang.org/pulse.html, seems like every package that uses mmap is timing out and being killed. Any ideas why this would cause such a regression?

[quinnj]

Drat, it looks like my deprecations got borked somehow to an older version; having to rebase the deprecations must have got tripped up. Ref #12070, which I'll merge when green.

[simonster]

This should probably go back to using pwrite, since that only had to write a single byte, whereas this is allocating and writing a potentially large array of zeros. It also doesn't seem that this seeks to the end of the file first, so it doesn't seem like it would yield the correct behavior if the file already exists?

[timholy]

For those of us migrating over to the new syntax, it would be great to have @compat Mmap.mmap(...) support for older versions of julia.

[quinnj]

For those wanting compat: JuliaLang/Compat.jl#123