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

Add a warning against having multiple copies of a shared library loaded #43007

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

petvana
Copy link
Member

@petvana petvana commented Nov 8, 2021

This PR aims to solve #42896 by reincarnation of #7490 originally created by @vtjnash.

Since PyCall loads libraries on the fly, it also need to be updated as well (see this PR). That is also the reason why check_dllist is exported.

@petvana petvana changed the title add a warning against having multiple copies of a shared library loaded Add a warning against having multiple copies of a shared library loaded Nov 8, 2021
@petvana petvana marked this pull request as draft November 8, 2021 22:52
@ararslan
Copy link
Member

ararslan commented Nov 9, 2021

IMO it would make more sense for this to be an optional, opt-in safety check built into dlopen rather than a separate function.

@petvana
Copy link
Member Author

petvana commented Nov 9, 2021

@ararslan Agree that the warning should be optional. However, this PR targets beginner users accidentally using two incompatible packages (like Gtk and PyPlot). Thus, IMHO, I'd enable it by default (opt-out), like Bounds checking in Julia.

It is not necessary to export new function, we can duplicate the code to PyCall for the case new libraries are loaded by pyimport and not by dlopen.

We can also suppress the warning if two exactly same files but from different locations are loaded (symlinks are already detected).

@petvana
Copy link
Member Author

petvana commented Nov 9, 2021

CI produces the following two warnings.

For macos64

┌ Warning: Detected possible duplicate library loaded: libunwind
│ This may lead to unexpected behavior!
│ /usr/lib/system/libunwind.dylib
│ /Users/julia/buildbot/worker-tabularasa/tester_macos64/build/lib/julia/libunwind.1.0.dylib
└ @ Base.Libc.Libdl libdl.jl:114

For freebsd64

┌ Warning: Detected possible duplicate library loaded: libatomic
│ This may lead to unexpected behavior!
│ /usr/home/julia/buildbot/w2_tester/tester_freebsd64/build/lib/julia/libatomic.so.3/usr/local/lib/gcc10/libatomic.so.1
└ @ Base.Libc.Libdl libdl.jl:114

Is it correct, or can be harmful? I cannot reproduce locally because I use Ubuntu.

@giordano
Copy link
Contributor

Have you measured the performance impact of this? When loading certain packages we may dlopen some dozens of libraries, even if a single check may not be too expensive, it all adds up, and you're going to iterate every time on a larger dllist

@petvana
Copy link
Member Author

petvana commented Nov 10, 2021

@giordano Excellent point. Right now, it seems negligible for average packages. However, the complexity is right now O(n n^2) = O(n^3) of string comparisons, where n is the number of loaded libs. By sorting, we can easily go down to O(n^2 log n). Do you have in mind an example of a package that loads a lot of libraries to be tested? In such a case, the specific package can opt-out and test duplicity only once when initialized.

@petvana
Copy link
Member Author

petvana commented Nov 10, 2021

EDITED: I have improved the performance by sorting a dictionary. So, now the complexity is O(n^2 log n) O(n^2). The time (of a single check) with 215 libraries was reduced from about 1.5 ms to 263.650 μs 39 μs. (Updated after Add comments and note to docs commit)

julia> using Gtk

julia> using PyPlot
┌ Warning: Detected possible duplicate library loaded: libffi
│ This may lead to unexpected behavior!
│ /home/petr/.julia/conda/3/lib/python3.7/lib-dynload/../../libffi.so.7/home/petr/.julia/artifacts/f1a54fe617cfa5997fa86c2cb15b93ac1d8c4c24/lib64/libffi.so
│ To suppress this warning, you can use the following argument:dlopen([name_or_path]; suppress_warnings = ["libffi"])
└ @ Base.Libc.Libdl libdl.jl:116
┌ Warning: Detected possible duplicate library loaded: libjpeg
│ This may lead to unexpected behavior!
│ /home/petr/.julia/conda/3/lib/python3.7/site-packages/PIL/../../../libjpeg.so.9/home/petr/.julia/artifacts/b9b138178f2c19d38aa8a19eac1c256f54bb6234/lib/libjpeg.so
│ To suppress this warning, you can use the following argument:dlopen([name_or_path]; suppress_warnings = ["libjpeg"])
└ @ Base.Libc.Libdl libdl.jl:116

julia> using BenchmarkTools

julia> using Libdl

julia> Libdl.dllist() |> length
214

julia> @btime dllist();
  14.440 μs (434 allocations: 39.23 KiB)

julia> @btime dlopen("")
  38.515 μs (449 allocations: 64.68 KiB)

@giordano
Copy link
Contributor

giordano commented Nov 10, 2021

Do you have in mind an example of a package that loads a lot of libraries to be tested?

Gtk.jl dlopens exactly 100 libraries on Linux according to my estimation. We've spent lot of time bringing the loading time of GTK3_jll under 10 ms, this PR would kill that.

In such a case, the specific package can opt-out and test duplicity only once when initialized.

That isn't really feasible. Libraries are dlopened by JLL packages, not by a single package.

@KristofferC
Copy link
Sponsor Member

Just time loading Gtk_jll before and after this PR to get the actual numbers?

@petvana
Copy link
Member Author

petvana commented Nov 10, 2021

Tested on Intel(R) Core(TM) i5-1035G1 CPU @ 1.00GHz (Updated after the Make PR thread-safe commit)
Master:

julia> @time using GTK3_jll
  0.108701 seconds (498.00 k allocations: 21.129 MiB, 26.65% compilation time)

PR:

julia> @time using GTK3_jll
  0.110910 seconds (527.88 k allocations: 23.245 MiB, 21.68% compilation time)

Tested on Intel(R) Core(TM) i7-9700 CPU @ 3.00GHz
Master:

julia> @time using GTK3_jll
  0.081210 seconds (474.67 k allocations: 20.426 MiB, 23.50% compilation time)

PR:

julia> @time using GTK3_jll
  0.083450 seconds (505.28 k allocations: 22.627 MiB, 20.80% compilation time)

(Most of the time is spend in dlname() calling basename() that can be optimized.)

base/libdl.jl Outdated
const dlpattern = r"^(.+?)\.so(?:\..*)?$"
end

const _dlname_cache = Dict{String, SubString{String}}()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not thread-safe and goes against #41602

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the comment; it should be thread-safe now (without a performance regression).

@petvana
Copy link
Member Author

petvana commented Nov 11, 2021

Function check_dllist() is not exported any more, and one can execute the check by Libdl.dlopen(""), like in JuliaPy/PyCall.jl#942

@petvana
Copy link
Member Author

petvana commented Nov 11, 2021

@giordano I've added an option to suppress any future warnings for a specific library, as the warning informs. Would it work for libpetsc?

julia> using PyCall

julia> using GTK3_jll
┌ Warning: Detected possible duplicate library loaded: libffi
│ This may lead to unexpected behavior!
│ /home/petr/.julia/artifacts/f1a54fe617cfa5997fa86c2cb15b93ac1d8c4c24/lib64/libffi.so
│ /home/petr/.julia/conda/3/lib/python3.8/lib-dynload/../../libffi.so.7
│ To suppress this warning, you can use the following argument:dlopen([name_or_path]; suppress_warnings = ["libffi"])
└ @ Base.Libc.Libdl libdl.jl:110

@petvana
Copy link
Member Author

petvana commented Nov 16, 2021

@vtjnash Would you please check this PR (as the author of the original idea)? I've improved performance to spend only about 2-3% when loading GTK3_jll (one of the worst cases).

@petvana petvana marked this pull request as ready for review November 16, 2021 09:56
@vtjnash vtjnash added the triage This should be discussed on a triage call label Nov 17, 2021
@JeffBezanson
Copy link
Sponsor Member

Triage is ok with this, as long as users won't be spammed with too many or duplicate warnings. Performance impact also needs to be negligible, which it looks like it is.

@petvana
Copy link
Member Author

petvana commented Nov 18, 2021

Wow, the triaging was fast, thanks.

First, it would be convenient to have Julia itself warning-free. The CI already found at least two possible duplicities (for macos64 and freebsd64 as mentioned above #43007 (comment)). Is it expected behavior? (Unfortunately, I don't use any of these OSes now.)

@vtjnash vtjnash added forget me not PRs that one wants to make sure aren't forgotten and removed triage This should be discussed on a triage call labels Dec 2, 2021
@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 2, 2021

triage noted we need to solve those existing warnings first

@JeffBezanson
Copy link
Sponsor Member

Yeah, if there are problems there we should fix them first, and also need to make sure the warning is 100% accurate or it will be a bad user experience. We don't want to train people to ignore warnings.

@petvana petvana marked this pull request as draft April 5, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forget me not PRs that one wants to make sure aren't forgotten
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants