-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Mmap fix on system with 64k pagesize. #59695
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
base: master
Are you sure you want to change the base?
Conversation
Seek file back to beginning before running next text in mmap.
Seemed to be 4096 on a system with 64k pagesize. Removing const allowed it to be re-evaluated at runtime.
stdlib/Mmap/src/Mmap.jl
Outdated
export mmap | ||
|
||
const PAGESIZE = Int(Sys.isunix() ? ccall(:jl_getpagesize, Clong, ()) : ccall(:jl_getallocationgranularity, Clong, ())) | ||
PAGESIZE = Int(Sys.isunix() ? ccall(:jl_getpagesize, Clong, ()) : ccall(:jl_getallocationgranularity, Clong, ())) |
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.
@vtjnash is
PAGESIZE = Int(Sys.isunix() ? ccall(:jl_getpagesize, Clong, ()) : ccall(:jl_getallocationgranularity, Clong, ())) | |
const PAGESIZE = OncePerProcess{Int}() do | |
Int(Sys.isunix() ? ccall(:jl_getpagesize, Clong, ()) : ccall(:jl_getallocationgranularity, Clong, ())) | |
end |
the right way to do this on master
?
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.
But then all references to PAGESIZE
should be changed to PAGESIZE()
. Apparently there are some packages which were using this internal symbol assuming it was a constant, and we'll need to update them: https://juliahub.com/ui/Search?type=code&q=Mmap.PAGESIZE
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.
I'm not certain you need the overhead of OncePerProcess to memoize the read of the getpagesize variable
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.
So this should just be a regular function?
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.
I opened #59712 as an alternative to this PR, to create a pagesize
function.
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.
Thanks for taking a look - I was looking eleswhere in the code and the Sys module which seems to use __init__
function to set similar type of variables. e.g. Sys module
Made me think whether PAGESIZE should be in Sys to keep these sorts of settings in the same location.
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.
Agreed, Sys.__init__
makes sense, then just needs using Sys: PAGESIZE
here
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.
Attempted to test this out or at least draft it.
Add PAGESIZE.
Export PAGESIZE
Use new PAGESIZE location
Not sure if this is the best approach but seemed to allow Mmap test to pass. Seems the pagesize was being set to value at build-time rather than being re-evaluated at runtime. Running Mmap tests on system with 64k pagesize, Mmap would still think its on a system with 4k pagesize and therefore fail.