fix Mmap.sync! for array mapped from an offset

[tanmaykm]

This fixes error when calling Mmap.sync! on an array that is not memory mapped from a pagesized offset of a file.

julia> using Base.Mmap

julia> f = open("arrayfile", "r+");

julia> A = Mmap.mmap(f, Vector{Int64}, (200,), 0);

julia> Mmap.sync!(A) #works

julia> B = Mmap.mmap(f, Vector{Int64}, (200,), 8);

julia> Mmap.sync!(B) #fails
ERROR: SystemError: msync: Invalid argument
 [inlined code] from ./int.jl:33
 in sync!(Base.Mmap.#sync!, Array{Int64,1}, Int64) at ./mmap.jl:207 (repeats 2 times)
 in eval at ./boot.jl:267

Since Mmap.mmap maps from the beginning of a page boundary, pointer(B) may not be at the page boundary when an offset is provided.

But since the pointer returned from mmap is also aligned at page boundary, sync! now recalculates the offset before calling msync!.

[yuyichao]

If you are going to add a test, make sure the offset is properly aligned (to the alignment of the element type) Ref #14551.

[tanmaykm]

Ah Ok. Updated the code snippet above. Will include a test also in a bit.

[tkelman]

IIRC the page size and allocation granularity are different on windows (edit: line 5)

definitely needs a test

[tkelman]

clean it up when finished

[tanmaykm]

done now

[tkelman]

eperm, might need to force a gc finalize? https://ci.appveyor.com/project/StefanKarpinski/julia/build/1.0.13049/job/mx2upgjkcduxik5v

[tanmaykm]

Hmm... yes, needed on Windows. Should have noticed it use elsewhere in the tests.
Done now. Thanks!

[tanmaykm]

AppVeyor tests failed.
Tests passed for mmap, but a subsequent test failed on the same worker.
@tkelman could this be causing some memory corruption on Windows as you suspected?

[tanmaykm]

No failures after a retry.
I don't have access to windows resources to do any further checks.
@tkelman ?

[tkelman]

I'll run this branch locally several times tomorrow and merge if I don't see any trouble. I think the limited memory on appveyor has been triggering all sorts of intermittent issues, especially since the llvm 3.7 and functions merges.

[quinnj]

Yeah, I think as long as you force GC after doing Mmap.mmap, that should be all that's necessary on windows. Windows holds some kind of reference to the file when it is mmapped, so we get in trouble if it doesn't get finalized before the file is removed or re-opened.

I'm a little confused on why this is needed; the pointer returned by mmap should be page-aligned, so I don't see how it would need to be page aligned later (see the code here: https://github.com/tanmaykm/julia/blob/fc9fd22181664b4c61c52dfcf973ab919529bf49/base/mmap.jl#L111). Obviously there was something you were hitting, so I'm not doubting there's a fix needed, I'm just not quite following what the issue is here.

[tanmaykm]

@quinnj when the array is being read from an offset that's not page aligned, the pointer to the array is not exactly the pointer that mmap returned. Therefore sync! needs to recalculate the right pointer value to call msync! with.

[tanmaykm]

Also, when the page is dirty, simple unmapping (which happens with gc) doesn't guarantee flushing and invalidation.

[tanmaykm]

I was able to get a Windows machine and test this locally.
It ran fine multiple times without errors or signs of memory corruption.
Will merge this in a while.

[quinnj]

Thanks for the response @tanmaykm, that makes sense. LGTM

[tanmaykm]

Thanks for validating @quinnj