-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix mmap_array with offset on Windows #7242
Conversation
query allocation granularity (64k on Windows, larger than page size which is 4k), ensure offset to MapViewOfFile is a multiple of the granularity add a test for mmap with offset
error("file is too large to memory-map on this platform") | ||
end | ||
# Set the offset to a page boundary | ||
offset_page::FileOffset = ifloor(offset/granularity)*granularity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use idiv instead?
Before merging, it will probably be useful to wait until JuliaIO/HDF5.jl#89 is working, as it's another easy way to test mmap facilities. |
szarray = convert(Csize_t, len) | ||
szfile = szarray + convert(Csize_t, offset) | ||
szfile = szarray + convert(Csize_t, offset-offset_page) | ||
mmaphandle = ccall(:CreateFileMappingW, stdcall, Ptr{Void}, (Ptr{Void}, Ptr{Void}, Cint, Cint, Cint, Ptr{Uint16}), | ||
shandle.handle, C_NULL, flprotect, szfile>>32, szfile&0xffffffff, C_NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from reading msdn, it appears that szfile here should = offset + szarray
or it can equal 0, without any actual cost (http://msdn.microsoft.com/en-us/library/windows/desktop/aa366542(v=vs.85).aspx)
replace 0xffffffff with typemax(Uint32) add a test with a larger offset than 64k use div(a, b) instead of ifloor(a/b)
Thanks @vtjnash, you clearly understand what you're reading on MSDN better than I do. Your recommended changes fixed |
I recently added support for printing error messages on windows (https://github.com/JuliaLang/julia/blob/master/base/env.jl#L14). It would be great if you could clean that up a bit and start using it everywhere we do calls to the windows API, so that we get better error messages. |
Ooh, I like:
that was by changing the error message to |
this looks good to merge. I can't quite follow the HDF5 issue, however, if that is still a problem. |
The HDF5 issue seems to be a combination of different permissions when Julia is run non-interactively (something fishy with environment variables in spawned processes maybe?), and |
That's definitely not an endianness issue; if it were, the result would be |
hm, okay. well the bytes are in reverse order in the .jld file, but that's true for both linux and windows hdf5... |
OK, the HDF5 problem has been debugged to the point that I suspect this can be merged. |
Fix mmap_array with offset on Windows
query allocation granularity (64k on Windows, larger than page size which is 4k),
ensure offset to MapViewOfFile is a multiple of the granularity
add a test for mmap_array with offset
fixes #7080 and #6203
some mmap issues remain with HDF5.jl, but there may be something else going on there