Skip to content

Commit

Permalink
Warn if an already loaded package is attempted to be loaded from a di…
Browse files Browse the repository at this point in the history 
…fferent path (#44329)
  • Loading branch information
@KristofferC
KristofferC committed Dec 23, 2023
1 parent 52ff558 commit b51b809
Showing 1 changed file with 41 additions and 0 deletions.
41 changes: 41 additions & 0 deletions base/loading.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,7 @@ function _tryrequire_from_serialized(modkey::PkgId, build_id::UInt128)
assert_havelock(require_lock)
loaded = nothing
if root_module_exists(modkey)
warn_if_already_loaded_different(modkey)
loaded = root_module(modkey)
else
loaded = start_loading(modkey)
Expand Down Expand Up @@ -1532,6 +1533,7 @@ function _tryrequire_from_serialized(modkey::PkgId, path::String, ocachepath::Un
assert_havelock(require_lock)
loaded = nothing
if root_module_exists(modkey)
warn_if_already_loaded_different(modkey)
loaded = root_module(modkey)
else
loaded = start_loading(modkey)
Expand Down Expand Up @@ -1975,11 +1977,50 @@ function __require_prelocked(uuidkey::PkgId, env=nothing)
# After successfully loading, notify downstream consumers
run_package_callbacks(uuidkey)
else
warn_if_already_loaded_different(uuidkey)
newm = root_module(uuidkey)
end
return newm
end

const already_warned_path_change_pkgs = Set{UUID}()
# warns if the loaded version of a module is different to the one that locate_package wants to load
function warn_if_already_loaded_different(uuidkey::PkgId)
uuidkey.uuid ∈ already_warned_path_change_pkgs && return
pkgorig = get(pkgorigins, uuidkey, nothing)
if pkgorig !== nothing && pkgorig.path !== nothing
new_path = locate_package(uuidkey)
if !samefile(fixup_stdlib_path(pkgorig.path), new_path)
if isnothing(pkgorig.version)
v = get_pkgversion_from_path(dirname(dirname(pkgorig.path)))
cur_vstr = isnothing(v) ? "" : "v$v "
else
cur_vstr = "v$v "
end
new_v = get_pkgversion_from_path(dirname(dirname(new_path)))
new_vstr = isnothing(new_v) ? "" : "v$new_v "
warnstr = """
$uuidkey is already loaded from a different path:
loaded: $cur_vstr$(repr(pkgorig.path))
requested: $new_vstr$(repr(new_path))
"""
if isempty(already_warned_path_change_pkgs)
warnstr *= """
This might indicate a change of environment has happened between package loads and can mean that
incompatible packages have been loaded"""
if JLOptions().startupfile < 2 #(0 = auto, 1 = yes)
warnstr *= """. If this happened due to a startup.jl consider starting julia
directly in the project via the `--project` arg."""
else
warnstr *= "."
end
end
@warn warnstr
push!(already_warned_path_change_pkgs, uuidkey.uuid)
end
end
end

mutable struct PkgOrigin
path::Union{String,Nothing}
cachepath::Union{String,Nothing}
Expand Down

7 comments on commit b51b809

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

The package evaluation job you requested has completed - no new issues were detected.
The full report is available.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

Are the sort changes expected (@LilithHafner)?

sort | 1.12 (20%) | 0.08 (1%) ✅
["sort", "issues", "partialsort(rand(10_000), 10_000)"] | 1.43 (20%) ❌ | 0.54 (1%) ✅
["sort", "issues", "sortslices sorting very short slices"] | 2.36 (20%) ❌ | 1.00 (1%)

@LilithHafner
Copy link
Member

Choose a reason for hiding this comment

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

I expect a change in partialsort performance due to #52494, and I expect it to be an improvement. Reproducing locally using Charmarks, I find an improvement:

Old

julia> using Chairmarks

julia> @be rand(10_000) partialsort(_, 10_000) seconds=2.0
Benchmark: 3746 samples with 1 evaluation
min    21.791 μs (6 allocs: 156.375 KiB)
median 32.000 μs (6 allocs: 156.375 KiB)
mean   691.230 μs (6 allocs: 156.375 KiB, 0.03% gc time)
max    2.449 s (6 allocs: 156.375 KiB, 97.85% gc time)

julia> @be rand(10_000) partialsort!(_, 1:3; rev=true) evals=1 seconds=2.0
Benchmark: 69081 samples with 1 evaluation
min    14.708 μs (3 allocs: 78.188 KiB)
median 19.792 μs (3 allocs: 78.188 KiB)
mean   21.456 μs (3 allocs: 78.188 KiB, 0.12% gc time)
max    885.088 μs (3 allocs: 78.188 KiB, 93.29% gc time)

julia> @be rand(Int, 30_000_000, 2) sortslices(_, dims=2) seconds=2.0
Benchmark: 21 samples with 1 evaluation
min    37.074 ms (5 allocs: 457.764 MiB, 0.69% gc time)
median 38.842 ms (5 allocs: 457.764 MiB, 0.76% gc time)
mean   39.909 ms (5 allocs: 457.764 MiB, 1.50% gc time)
max    60.497 ms (5 allocs: 457.764 MiB, 7.08% gc time)

julia> versioninfo()
Julia Version 1.11.0-DEV.1101
Commit ad2d770e91 (2023-12-14 19:18 UTC)
Platform Info:
  OS: Linux (aarch64-redhat-linux)
  CPU: 8 × unknown
  WORD_SIZE: 64
  LLVM: libLLVM-15.0.7 (ORCJIT, generic)
  Threads: 1 on 8 virtual cores
Environment:
  JULIA_EDITOR = code

New

julia> using Chairmarks

julia> @be rand(10_000) partialsort(_, 10_000) seconds=2.0
Benchmark: 71827 samples with 1 evaluation
min    14.458 μs (9 allocs: 84.062 KiB)
median 16.667 μs (9 allocs: 86.938 KiB)
mean   18.225 μs (9 allocs: 87.008 KiB, 0.42% gc time)
max    8.352 ms (9 allocs: 91.688 KiB, 95.62% gc time)

julia> @be rand(10_000) partialsort!(_, 1:3; rev=true) evals=1 seconds=2.0
Benchmark: 92979 samples with 1 evaluation
min    11.958 μs (5 allocs: 5.703 KiB)
median 13.417 μs (6 allocs: 8.750 KiB)
mean   13.586 μs (6.00 allocs: 8.821 KiB)
max    53.917 μs (6 allocs: 13.438 KiB)

julia> @be rand(Int, 30_000_000, 2) sortslices(_, dims=2) seconds=2.0
Benchmark: 21 samples with 1 evaluation
min    37.727 ms (5 allocs: 457.764 MiB, 0.75% gc time)
median 39.178 ms (5 allocs: 457.764 MiB, 0.78% gc time)
mean   39.472 ms (5 allocs: 457.764 MiB, 1.54% gc time)
max    49.486 ms (5 allocs: 457.764 MiB, 16.65% gc time)

julia> versioninfo()
Julia Version 1.11.0-DEV.1164
Commit e96c13aa5b (2023-12-28 12:42 UTC)
Platform Info:
  OS: Linux (aarch64-redhat-linux)
  CPU: 8 × unknown
  WORD_SIZE: 64
  LLVM: libLLVM-15.0.7 (ORCJIT, generic)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)
Environment:
  JULIA_EDITOR = code

With partialsort, using the minimum runtime with a large selection of rng seeds is not representative, and it is significantly less representative for the old algorithm than the new algorithm, leading to under reported runtimes for the old algorithm. With partial scratch quick sort—and ordinary partial quick sort—if the first partition is lucky then it only takes a single pass through the whole data set and all subsequent passes cover only a very small fraction, while on average it takes 1 full pass, 1 half pass, 1 quarter pass, etc. In contrast, BracketedSort uses a random sample to pick a really good first (and only) pivot (called a signpost) and always (>99% of the time) only needs one pass. This is reflected empirically with a closer mean/median and min time

julia> @benchmark partialsort(x, 10_000) setup=(x=rand(10_000)) # Old
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min … max):  16.167 μs …  1.204 ms  ┊ GC (min … max): 0.00% … 97.30%
 Time  (median):     23.251 μs              ┊ GC (median):    0.00%
 Time  (mean ± σ):   24.994 μs ± 25.139 μs  ┊ GC (mean ± σ):  4.19% ±  4.26%

       ▂▅▄▂▅▆▅█▇▅▇▆▄▅▆▅▅▄▃▄▄▂▂▁▁                               
  ▁▁▁▃████████████████████████████▇▇▆▅▆▅▅▄▄▄▄▃▃▃▂▃▂▂▂▂▁▂▂▂▁▁▂ ▅
  16.2 μs         Histogram: frequency by time        37.5 μs <

 Memory estimate: 156.38 KiB, allocs estimate: 6.

julia> @benchmark partialsort(x, 10_000) setup=(x=rand(10_000)) # New
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min … max):  15.250 μs …  1.043 ms  ┊ GC (min … max): 0.00% … 94.16%
 Time  (median):     18.958 μs              ┊ GC (median):    0.00%
 Time  (mean ± σ):   19.862 μs ± 25.621 μs  ┊ GC (mean ± σ):  5.18% ±  4.37%

         ▃▅▂▄▂▂          ▃▁▆▇▅█▅▇▂▃▁                           
  ▁▁▂▃▄█▇████████▆▇▇▆▆▆█▇███████████▆▇▄▄▃▄▃▄▄▃▃▃▃▂▃▂▂▂▁▂▂▁▁▁▂ ▄
  15.2 μs         Histogram: frequency by time          24 μs <

 Memory estimate: 84.19 KiB, allocs estimate: 9.

An alternative, and more concerning explanation is that I optimized Bracketed Sort on my hardware and it doesn't generalize to the nanosoldier machines while the old algorithm does generalize. That would also explain why I can't reproduce the regression locally. Someone with different hardware would need to try to reproduce this locally using the median or mean metric to test that hypothesis.

Sortslices should not have changed. The benchmark looks a bit noisy on my machine. It is usually pretty consistent, but occasionally has extreme outliers.

julia> versioninfo()
Julia Version 1.11.0-DEV.1101
Commit ad2d770e91 (2023-12-14 19:18 UTC)
Platform Info:
  OS: Linux (aarch64-redhat-linux)
  CPU: 8 × unknown
  WORD_SIZE: 64
  LLVM: libLLVM-15.0.7 (ORCJIT, generic)
  Threads: 1 on 8 virtual cores
Environment:
  JULIA_EDITOR = code

julia> a_9832 = rand(Int, 30_000_000, 2);

julia> @btime sortslices($a_9832, dims=2);
  2.854 s (5 allocations: 457.76 MiB)

julia>
x@fedora:~/.julia/dev/julia2$ ./julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.11.0-DEV.1101 (2023-12-14)
 _/ |\__'_|_|_|\__'_|  |  Commit ad2d770e91 (14 days old master)
|__/                   |


julia> a_9832 = rand(Int, 30_000_000, 2);

julia> @btime sortslices($a_9832, dims=2);
  35.435 ms (5 allocations: 457.76 MiB)

julia> versioninfo()
Julia Version 1.11.0-DEV.1101
Commit ad2d770e91 (2023-12-14 19:18 UTC)
Platform Info:
  OS: Linux (aarch64-redhat-linux)
  CPU: 8 × unknown
  WORD_SIZE: 64
  LLVM: libLLVM-15.0.7 (ORCJIT, generic)
  Threads: 1 on 8 virtual cores
Environment:
  JULIA_EDITOR = code

I suspect the extreme outliers are do to high memory pressure elsewhere in the system forcing swap space usage.

Conclusion

  • I don't think the regressions are real, but it wouldn't hurt for someone with different hardware to also try to reproduce them
  • The methodology for partialsort benchmarks was bad but is now reasonable (because minimum is not fairly reflective of mean) so need not be changed
  • Maybe we should use a smaller input for the sortslices test to avoid soft OOM

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on b51b809 Jan 4, 2024

Choose a reason for hiding this comment

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

high memory pressure elsewhere in the system

The system has 32 GB of dedicate RAM for the test. But sometimes benchmarks are just noisy anyways.

Please sign in to comment.