Skip to content
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

Don't permalloc the pkgimgs unnecessarily #48215

Closed
wants to merge 6 commits into from

Conversation

gbaraldi
Copy link
Member

It seems we were permallocing the memory for the sysimg even though the OS already does that more efficiently.

@vchuravy
Copy link
Member

I think we still hit this codepath with --pkgimages=no where we still would need to do that?

@gbaraldi
Copy link
Member Author

Yeah, annoyingly we don't have that information from there. Do you find it reasonable to check options there?

@vchuravy
Copy link
Member

@gbaraldi I pushed a fix. Can I ask you to write some tests?

@gbaraldi
Copy link
Member Author

So I was thinking about the test. Do we spawn a process with pkgimgs=no and load some package? Maybe a stdlib?

src/staticdata.c Outdated Show resolved Hide resolved
src/staticdata.c Outdated Show resolved Hide resolved
@vchuravy vchuravy added this to the 1.9 milestone Jan 12, 2023
@vchuravy vchuravy added the backport 1.9 Change should be backported to release-1.9 label Jan 12, 2023
@KristofferC KristofferC mentioned this pull request Jan 13, 2023
41 tasks

Verified

This commit was signed with the committer’s verified signature.
vchuravy Valentin Churavy
Co-authored-by: Valentin Churavy <v.churavy@gmail.com>
@vchuravy vchuravy force-pushed the dont-permalloc-pkgimg branch from 8eaa163 to ac5aed8 Compare January 16, 2023 13:38

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool/true/false is C++, this should use int/1/0

Error in testset precompile:
Error During Test at /cache/build/default-amdci4-3/julialang/julia-master/julia-bdc77d5c19/share/julia/test/precompile.jl:339
  Test threw exception
  Expression: begin
    m = Base._require_from_serialized(Base.PkgId(Foo), cachefile, ocachefile)
    #= /cache/build/default-amdci4-3/julialang/julia-master/julia-bdc77d5c19/share/julia/test/precompile.jl:341 =# @test m isa Module
end
  Error reading package image file.

@gbaraldi
Copy link
Member Author

Doesn't stdbool.h add bool/true/false?

@KristofferC KristofferC mentioned this pull request Jan 17, 2023
35 tasks

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@gbaraldi
Copy link
Member Author

gbaraldi commented Jan 20, 2023

I mentioned this privately to valentin. But the test failure we see there happens because we specifically load the same package image twice. Though since we are dlopening it, instead of mmaping a different copy, we just get the same thing, that means we have already performed relocations over it. So that test is doing something kinda illegal here. Though we might want to add a check that we've already loaded a sysimg before.

@KristofferC Do we have a check to see if package was loaded beforehand? Or does it happen on a higher level than

julia/base/loading.jl

Lines 1763 to 1773 in 57101cf

function _require_from_serialized(uuidkey::PkgId, path::String, ocachepath::Union{String, Nothing})
@lock require_lock begin
set_pkgorigin_version_path(uuidkey, nothing)
newm = _tryrequire_from_serialized(uuidkey, path, ocachepath)
newm isa Module || throw(newm)
insert_extension_triggers(uuidkey)
# After successfully loading, notify downstream consumers
run_package_callbacks(uuidkey)
return newm
end
end

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was signed with the committer’s verified signature.
gbaraldi Gabriel Baraldi
It loads the same package image twice, which triggers a checksum failure.
test/precompile.jl Outdated Show resolved Hide resolved

Unverified

This user has not yet uploaded their public signing key.
@KristofferC
Copy link
Member

Do we have a check to see if package was loaded beforehand?

The check is typically at

if !root_module_exists(uuidkey)
but if you call require directly then that is skipped and the existing module is overwritten.

@vchuravy
Copy link
Member

vchuravy commented Feb 1, 2023

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@gbaraldi
Copy link
Member Author

gbaraldi commented Feb 7, 2023

@vchuravy I was talking with @vtjnash and maybe we want to move the data section back to the .ji files. Two reasons 1, we might want to load the same file twice in order to inspect it before relocations, which dlopen doesn't allow. The second thing is that controlling how we mmap the files means we can maybe use huge pages in their load to save a nice amount of time, since julia boot time is mostly page faults.

@vchuravy
Copy link
Member

vchuravy commented Feb 7, 2023

Sure that sounds reasonable. I won't have time to tackle it though.

@KristofferC KristofferC removed this from the 1.9 milestone Feb 14, 2023
@KristofferC
Copy link
Member

Doesn't make sense really for this to be on the milestone imo.

@gbaraldi
Copy link
Member Author

It was on the milestone because it was initially a small fix, but on further discussion it's probably not so simple. So I think we can release 1.9 without it. If the fix isn't too difficult we can probably land it on a 1.9 patch

@KristofferC KristofferC mentioned this pull request Feb 20, 2023
50 tasks
@KristofferC KristofferC mentioned this pull request Mar 7, 2023
52 tasks
@KristofferC KristofferC mentioned this pull request Apr 9, 2023
26 tasks
@KristofferC KristofferC mentioned this pull request May 8, 2023
51 tasks
@vchuravy
Copy link
Member

From the HPC call: This currently breaks PkgInspector. So the idea is to add an opt-out env variable that users of PkgInspector can set to get the copy back.

@vchuravy vchuravy closed this May 25, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants