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

only store version of packages during load time when generating sysimage #46738

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

KristofferC
Copy link
Member

I am looking at the number of stat calls during code loading (precompilation). One somewhat significant source of these is the lookup of the version field in the project file of packages (added in #44318) that happens as soon as a package is loaded. Due to the way the precompilation process work (IIUC), the number of these lookup scales quadratically with the depth of the dependency tree of a package.

This PR changes it so that the lookup only happens when a call to pkgversion actually requests the version. An exception is when a sysimage is generated (for example from PackageCompiler) because then we want to also store the version to support the functionality in JuliaLang/Pkg.jl#3002.

The script I am using to measure the number of stat calls is:

❯ rm -rf ~/.julia/compiled/v1.9;  strace -f -o stat_gr~/juliaarray/julia --project -e 'using GR_jll'

❯ cat stat_gr | grep -c statx

executed in an environment with only GR_jll installed. The results are

❯ cat stat_master | grep -c statx
23789
❯ cat stat_master | grep -c statx
21236

This may not seem like a huge reduction but further work on #46690 brings the baseline down heavily where this absolute difference is quite significant.

@timholy
Copy link
Member

timholy commented Sep 13, 2022

Due to the way the precompilation process work (IIUC), the number of these lookup scales quadratically with the depth of the dependency tree of a package.

Couldn't this be solved by storing the version in the .ji header?

@KristofferC
Copy link
Member Author

Couldn't this be solved by storing the version in the .ji header?

That probably also works. I am not sure I see a big advantage doing that over this PR though.

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.

LGTM. But didn't we already have to parse this TOML file as part of picking the package to load?

@KristofferC
Copy link
Member Author

But didn't we already have to parse this TOML file as part of picking the package to load?

I don't think so. All that is required is the current Project + Manifest to compute what is loaded.

@KristofferC KristofferC merged commit b1f8d16 into master Sep 14, 2022
@KristofferC KristofferC deleted the kc/stat_version branch September 14, 2022 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants