Skip to content

Convert remaining JLL stdlibs to LazyLibraries #58444

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

Merged
merged 2 commits into from
May 28, 2025

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented May 17, 2025

Builds on #58405 (which I rebased, so the merge base here is master for now)

TODO:

@topolarity
Copy link
Member

Unfortunately the --trim failure is expected until we resolve #57707:

Core.MethodError(f=Base.Libc.Libdl.var"#dlsym"(), args=("/cache/julia-buildkite-plugin/depots/53de1ea5-6dd3-4008-b976-161c9f34b0ad/artifacts/4db1e58d71ac6bbb35ecc832033264c630d5d3b3/lib/libzstd.so", :ZSTD_versionString), world=0x00000000000097b7)

(LinearAlgebra is already broken for --trim on master for the same reason)

We do have a plan to fix it, but I don't know when it's going to happen since @Keno would like to revamp our ccall semantics (#57931) around the same time

@IanButterworth
Copy link
Member Author

IanButterworth commented May 17, 2025

Ah ok.
In its current state, along with JuliaSparse/SparseArrays.jl#626 this PR gives

macOS M2 compared to results in #58436

julia> @time using Plots
  0.850111 seconds (1.29 M allocations: 74.658 MiB, 6.23% gc time, 2.34% compilation time)

Linux AMD Ryzen 9 5950X

julia> @time using Plots
  0.757390 seconds (1.38 M allocations: 77.337 MiB, 4.01% gc time, 3.59% compilation time)

And basic startup appears to be faster (if that makes sense?)
master vs. PR

% hyperfine './julia --start=no -e "1+1"' '/Users/ian/.julia/juliaup/julia-nightly/bin/julia --start=no -e "1+1"'
Benchmark 1: ./julia --start=no -e "1+1"
  Time (mean ± σ):      89.3 ms ±   3.6 ms    [User: 58.6 ms, System: 29.0 ms]
  Range (min … max):    85.4 ms …  98.2 ms    30 runs

Benchmark 2: /Users/ian/.julia/juliaup/julia-nightly/bin/julia --start=no -e "1+1"
  Time (mean ± σ):      99.2 ms ±   2.1 ms    [User: 67.8 ms, System: 29.7 ms]
  Range (min … max):    97.0 ms … 107.3 ms    27 runs

Summary
  ./julia --start=no -e "1+1" ran
    1.11 ± 0.05 times faster than /Users/ian/.julia/juliaup/julia-nightly/bin/julia --start=no -e "1+1"

@@ -3,6 +3,6 @@
using Test, Libdl, GMP_jll

@testset "GMP_jll" begin
vn = VersionNumber(unsafe_string(unsafe_load(cglobal((:__gmp_version, libgmp), Ptr{Cchar}))))
vn = VersionNumber(unsafe_string(unsafe_load(cglobal(dlsym(libgmp, :__gmp_version), Ptr{Cchar}))))
Copy link
Member Author

Choose a reason for hiding this comment

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

Should/could cglobal be taught how to handle a LazyLibrary?

Copy link
Member

Choose a reason for hiding this comment

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

It could, the most annoying thing is just that cglobal() is an intrinsic, not a function, so to add an overload for that involves writing C code, not just Julia code.

@IanButterworth IanButterworth added the latency Latency label May 17, 2025
@IanButterworth IanButterworth force-pushed the ib/more_lazy branch 4 times, most recently from 7fb4a84 to 46fe088 Compare May 17, 2025 12:19
@topolarity
Copy link
Member

Can / should we add a test that checks the dependency list against the ldd (or DT_NEEDED entries) of the .so's?

@IanButterworth

This comment was marked as resolved.

@vtjnash
Copy link
Member

vtjnash commented May 17, 2025

BinaryBuilder should already audit that, except for a few know broken libraries

@topolarity
Copy link
Member

BinaryBuilder should already audit that, except for a few know broken libraries

Right, but it would be good to make sure that we haven't fallen out-of-sync with BinaryBuilder

@staticfloat
Copy link
Member

it would be good to make sure that we haven't fallen out-of-sync with BinaryBuilder

I agree, I added a test that can be run on macOS or Linux. I (or someone else feeling brave) can make one for Windows as well. It already catches plenty of minor issues:

┌ Warning: Dependency mismatch
│   jll = "SuiteSparse_jll"
│   library = "libbtf"
│   missing_deps = ""
│   extraneous_deps = "libsuitesparseconfig, libamd, libcolamd"
└ @ Main ~/src/julia/test/stdlib_dependencies.jl:169

┌ Warning: Dependency mismatch
│   jll = "SuiteSparse_jll"
│   library = "libldl"
│   missing_deps = ""
│   extraneous_deps = "libsuitesparseconfig"
└ @ Main ~/src/julia/test/stdlib_dependencies.jl:169

┌ Warning: Dependency mismatch
│   jll = "SuiteSparse_jll"
│   library = "libspqr"
│   missing_deps = "libstdc++, libgcc_s"
│   extraneous_deps = "libamd, libcolamd"
└ @ Main ~/src/julia/test/stdlib_dependencies.jl:169

┌ Warning: Dependency mismatch
│   jll = "SuiteSparse_jll"
│   library = "libumfpack"
│   missing_deps = ""
│   extraneous_deps = "libcolamd"
└ @ Main ~/src/julia/test/stdlib_dependencies.jl:169

┌ Warning: Dependency mismatch
│   jll = "MPFR_jll"
│   library = "libmpfr"
│   missing_deps = ""
│   extraneous_deps = "libgmpxx"
└ @ Main ~/src/julia/test/stdlib_dependencies.jl:169
Test Summary:               | Pass  Fail  Total  Time
Stdlib JLL dependency check |   29     5     34  3.9s
RNG of the outermost testset: Random.Xoshiro(0x1329e9815a60ab0e, 0x784c9414423a89d0, 0x1a3aea28fda405f6, 0x8c1dfb6d4dda4f7b, 0xd4081888363c92a9)
ERROR: LoadError: Some tests did not pass: 29 passed, 5 failed, 0 errored, 0 broken.
in expression starting at /home/sabae/src/julia/test/stdlib_dependencies.jl:132

Note that there are some slight differences between macOS and Linux, so you'll have to add some conditionals for those things.

@IanButterworth
Copy link
Member Author

Nice

@vtjnash
Copy link
Member

vtjnash commented May 19, 2025

Blocked by --trim failure

That should not actually be blocking. The required code dependencies should be vendored into the test directory anyways, rather than relying on changeable state elsewhere, since otherwise we can lose test coverage simply if these package files change, which should not be true of test code.

@staticfloat
Copy link
Member

The required code dependencies should be vendored into the test directory anyways, rather than relying on changeable state elsewhere, since otherwise we can lose test coverage simply if these package files change, which should not be true of test code.

I don't quite follow what you mean, can you be more explicit?

@IanButterworth IanButterworth force-pushed the ib/more_lazy branch 3 times, most recently from a415b58 to 488c8e9 Compare May 20, 2025 21:38
@IanButterworth IanButterworth force-pushed the ib/more_lazy branch 2 times, most recently from cdbfd6e to 7bf2956 Compare May 22, 2025 19:08
@IanButterworth IanButterworth force-pushed the ib/more_lazy branch 3 times, most recently from 1afe38a to 3a9548b Compare May 27, 2025 16:57
@IanButterworth IanButterworth added the merge me PR is reviewed. Merge when all tests are passing label May 28, 2025
@IanButterworth IanButterworth added backport 1.12 Change should be backported to release-1.12 and removed merge me PR is reviewed. Merge when all tests are passing labels May 28, 2025
staticfloat and others added 2 commits May 28, 2025 12:25
This converts more JLL stdlibs to use lazily-loaded libraries, and as a
useful side-effect, causes them to be loaded by absolute path, isolating
them all from `LD_LIBRARY_PATH`-like effects.

Adds tests that audits the expressed library dependencies in our
LazyLibrary JLL definitions, to ensure that we don't get out of sync
with the actual binaries.

Co-Authored-By: Mosè Giordano <[email protected]>
Co-Authored-By: Elliot Saba <[email protected]>
comment-out bad ccall with a fixme

Revert "comment-out bad ccall with a fixme"

This reverts commit 6fc66a2.

Revert "copy libzstd over for trimming tests"

This reverts commit dc4c2dc.

vendor the old Zstd_jll
@IanButterworth
Copy link
Member Author

I haven't figured out why, but load time seems to have regressed on this since #58444 (comment) (0.85s)

julia> @time using Plots
  0.969735 seconds (1.35 M allocations: 76.738 MiB, 5.62% gc time, 1.59% compilation time)

Lots of big changes on master since. We need visible key performance metrics like load time on every PR, before merge.

@IanButterworth IanButterworth merged commit e984c57 into JuliaLang:master May 28, 2025
7 checks passed
@IanButterworth IanButterworth deleted the ib/more_lazy branch May 28, 2025 20:41
@fingolfin fingolfin mentioned this pull request May 31, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12 JLLs latency Latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants