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

enable re-using external code in pkgimages #48723

Merged
merged 6 commits into from
Feb 25, 2023
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 19, 2023

This was unintentionally disabled (and incomplete) in the original PR for pkgimages. Hopefully this will cut down somewhat on pkgimage build times and sizes. There is still some broken dead code remaining here, which should be evaluate for whether it is actually needed.

@vtjnash vtjnash added compiler:latency Compiler latency backport 1.9 Change should be backported to release-1.9 labels Feb 19, 2023
@vtjnash vtjnash requested a review from vchuravy February 19, 2023 14:04
@KristofferC KristofferC mentioned this pull request Feb 20, 2023
50 tasks
@KristofferC
Copy link
Member

I tried Plots on this PR and I got:

julia> using Plots
[ Info: Precompiling Plots [91a5bcdd-55d7-5caf-9e0b-520d859cae80]
[ Info: GR

[930055] signal (11.1): Segmentation fault
in expression starting at none:1
unknown function (ip: (nil))
Allocations: 40409048 (Pool: 40385011; Big: 24037); GC: 58
ERROR: Failed to precompile Plots [91a5bcdd-55d7-5caf-9e0b-520d859cae80] to "/tmp/depot/compiled/v1.10/Plots/jl_XfbSKB".

Note that I haven't tried it on vanilla master so it might not be due to anything in here but thought I should mention it.

src/aotcompile.cpp Outdated Show resolved Hide resolved
src/codegen.cpp Outdated Show resolved Hide resolved
src/codegen.cpp Outdated Show resolved Hide resolved
@vchuravy
Copy link
Member

@nanosoldier runtests(configuration = (buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],), vs_configuration = (buildflags = ["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],))

Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

Great simplification. Still need to figure out what to do with !specsig,

@vchuravy
Copy link
Member

I tried Plots on this PR and I got:

julia> using Plots
[ Info: Precompiling Plots [91a5bcdd-55d7-5caf-9e0b-520d859cae80]
[ Info: GR

[930055] signal (11.1): Segmentation fault
in expression starting at none:1
unknown function (ip: (nil))
Allocations: 40409048 (Pool: 40385011; Big: 24037); GC: 58
ERROR: Failed to precompile Plots [91a5bcdd-55d7-5caf-9e0b-520d859cae80] to "/tmp/depot/compiled/v1.10/Plots/jl_XfbSKB".

Note that I haven't tried it on vanilla master so it might not be due to anything in here but thought I should mention it.

julia> using Plots
julia: /home/vchuravy/src/julia/src/staticdata.c:1944: jl_root_new_gvars: Assertion `codeinst && codeinst->isspecsig' failed.

[1428795] signal (6.-6): Aborted
in expression starting at REPL[4]:1
unknown function (ip: 0x7fe272a4e4dc)
gsignal at /usr/lib/libc.so.6 (unknown line)
abort at /usr/lib/libc.so.6 (unknown line)
unknown function (ip: 0x7fe2729e845b)
__assert_fail at /usr/lib/libc.so.6 (unknown line)
jl_root_new_gvars at /home/vchuravy/src/julia/src/staticdata.c:1944
jl_restore_system_image_from_stream_ at /home/vchuravy/src/julia/src/staticdata.c:3131
jl_restore_package_image_from_stream at /home/vchuravy/src/julia/src/staticdata.c:3254
jl_restore_incremental_from_buf at /home/vchuravy/src/julia/src/staticdata.c:3300
ijl_restore_package_image_from_file at /home/vchuravy/src/julia/src/staticdata.c:3413
_include_from_serialized at ./loading.jl:1005
_require_search_from_serialized at ./loading.jl:1431
_require at ./loading.jl:1764
_require_prelocked at ./loading.jl:1644
macro expansion at ./loading.jl:1630 [inlined]
macro expansion at ./lock.jl:267 [inlined]
require at ./loading.jl:1593
jfptr_require_54786 at /home/vchuravy/builds/julia-dbg/usr/lib/julia/sys-debug.so (unknown line)
_jl_invoke at /home/vchuravy/src/julia/src/gf.c:2672
ijl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2873
jl_apply at /home/vchuravy/src/julia/src/julia.h:1875
call_require at /home/vchuravy/src/julia/src/toplevel.c:466
eval_import_path at /home/vchuravy/src/julia/src/toplevel.c:503
jl_toplevel_eval_flex at /home/vchuravy/src/julia/src/toplevel.c:728
jl_toplevel_eval_flex at /home/vchuravy/src/julia/src/toplevel.c:853
ijl_toplevel_eval at /home/vchuravy/src/julia/src/toplevel.c:919
ijl_toplevel_eval_in at /home/vchuravy/src/julia/src/toplevel.c:969
eval at ./boot.jl:370 [inlined]
eval_user_input at /home/vchuravy/builds/julia-dbg/usr/share/julia/stdlib/v1.10/REPL/src/REPL.jl:149
repl_backend_loop at /home/vchuravy/builds/julia-dbg/usr/share/julia/stdlib/v1.10/REPL/src/REPL.jl:245
#start_repl_backend#46 at /home/vchuravy/builds/julia-dbg/usr/share/julia/stdlib/v1.10/REPL/src/REPL.jl:230
start_repl_backend at /home/vchuravy/builds/julia-dbg/usr/share/julia/stdlib/v1.10/REPL/src/REPL.jl:227
jl_fptr_args at /home/vchuravy/src/julia/src/gf.c:2333
_jl_invoke at /home/vchuravy/src/julia/src/gf.c:2672
ijl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2873
#run_repl#59 at /home/vchuravy/builds/julia-dbg/usr/share/julia/stdlib/v1.10/REPL/src/REPL.jl:373
run_repl at /home/vchuravy/builds/julia-dbg/usr/share/julia/stdlib/v1.10/REPL/src/REPL.jl:359
jfptr_run_repl_61344 at /home/vchuravy/builds/julia-dbg/usr/lib/julia/sys-debug.so (unknown line)
_jl_invoke at /home/vchuravy/src/julia/src/gf.c:2672
ijl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2873
#985 at ./client.jl:421
jfptr_YY.985_56124 at /home/vchuravy/builds/julia-dbg/usr/lib/julia/sys-debug.so (unknown line)
_jl_invoke at /home/vchuravy/src/julia/src/gf.c:2672
ijl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2873
jl_apply at /home/vchuravy/src/julia/src/julia.h:1875
jl_f__call_latest at /home/vchuravy/src/julia/src/builtins.c:778
#invokelatest#2 at ./essentials.jl:828 [inlined]
invokelatest at ./essentials.jl:825 [inlined]
run_main_repl at ./client.jl:405
exec_options at ./client.jl:322
_start at ./client.jl:541
jfptr__start_56130 at /home/vchuravy/builds/julia-dbg/usr/lib/julia/sys-debug.so (unknown line)
_jl_invoke at /home/vchuravy/src/julia/src/gf.c:2672
ijl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2873
jl_apply at /home/vchuravy/src/julia/src/julia.h:1875
true_main at /home/vchuravy/src/julia/src/jlapi.c:573
jl_repl_entrypoint at /home/vchuravy/src/julia/src/jlapi.c:717
jl_load_repl at /home/vchuravy/src/julia/cli/loader_lib.c:543
main at /home/vchuravy/src/julia/cli/loader_exe.c:58
unknown function (ip: 0x7fe2729e92cf)
__libc_start_main at /usr/lib/libc.so.6 (unknown line)
_start at /build/glibc/src/glibc/csu/../sysdeps/x86_64/start.S:115
Allocations: 10219305 (Pool: 10209417; Big: 9888); GC: 16

Will investigate

@vchuravy vchuravy force-pushed the jn/pkgimg-reuse-external branch from 5e307b8 to dabb822 Compare February 20, 2023 23:15
@vchuravy
Copy link
Member

@nanosoldier runtests(configuration = (buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],), vs_configuration = (buildflags = ["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],))

@nanosoldier
Copy link
Collaborator

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

@vchuravy
Copy link
Member

no new issues were detected.

@maleadt the report has 112 new crashes...

@vchuravy
Copy link
Member

Failed to precompile FreeTypeAbstraction [663a7486-cb36-511b-a19d-713bb74d65c9] to "/home/pkgeval/.julia/compiled/v1.10/FreeTypeAbstraction/jl_XX4CRD".
julia: /source/src/staticdata_utils.c:1060: jl_insert_backedges: Assertion `ptrhash_get(&visited, (void*)ci->def) == ((void*)1)' failed.

[9] signal (6.-6): Aborted
in expression starting at /home/pkgeval/.julia/packages/FreeTypeAbstraction/b8EQ2/src/FreeTypeAbstraction.jl:3
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7f139ac2640e)
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
jl_insert_backedges at /source/src/staticdata_utils.c:1060 [inlined]
jl_restore_package_image_from_stream at /source/src/staticdata.c:3263
ijl_restore_incremental at /source/src/staticdata.c:3313
_include_from_serialized at ./loading.jl:1008
_require_search_from_serialized at ./loading.jl:1431
_require at ./loading.jl:1764

@maleadt
Copy link
Member

maleadt commented Feb 21, 2023

@maleadt the report has 112 new crashes...

For packages that also failed in the comparison run, albeit for different reasons (create keyring LinearSolve-against-0OpNOo6z: Disk quota exceeded, this error is new to me).

@nanosoldier
Copy link
Collaborator

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

@maleadt
Copy link
Member

maleadt commented Feb 21, 2023

Let's try that again:

@nanosoldier runtests(configuration = (buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],), vs_configuration = (buildflags = ["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],))

@nanosoldier
Copy link
Collaborator

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

@KristofferC
Copy link
Member

467 packages crashed during testing on the previous version too.

😨

@maleadt
Copy link
Member

maleadt commented Feb 21, 2023

Looks like we haven't tried enabling assertions for a while... They all fail with:

Failed to precompile WGPUCompute [494b1f74-06dd-4437-8477-4a86f2a8d703] to "/home/pkgeval/.julia/compiled/v1.10/WGPUCompute/jl_ZbaSW4".
julia: /source/src/staticdata_utils.c:1060: jl_insert_backedges: Assertion `ptrhash_get(&visited, (void*)ci->def) == ((void*)1)' failed.

[30] signal (6.-6): Aborted
in expression starting at /home/pkgeval/.julia/packages/WGSLTypes/kOXzf/src/builtins.jl:1
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7fa8ea33040e)
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
jl_insert_backedges at /source/src/staticdata_utils.c:1060 [inlined]
jl_restore_package_image_from_stream at /source/src/staticdata.c:3264
ijl_restore_incremental at /source/src/staticdata.c:3314

vtjnash added a commit that referenced this pull request Feb 21, 2023
Previously we would double-account for many of these, leading to
occasional chaos. Try to avoid serializing code that does not belong to
this incremental compilation session package.

Refs: #48723
src/codegen.cpp Outdated Show resolved Hide resolved
vtjnash added a commit that referenced this pull request Feb 21, 2023
Previously we would double-account for many of these, leading to
occasional chaos. Try to avoid serializing code that does not belong to
this incremental compilation session package.

Refs: #48723
vtjnash added a commit that referenced this pull request Feb 21, 2023
Previously we would double-account for many of these, leading to
occasional chaos. Try to avoid serializing code that does not belong to
this incremental compilation session package.

Refs: #48723
@vchuravy vchuravy force-pushed the jn/pkgimg-reuse-external branch from dabb822 to ae27e81 Compare February 22, 2023 00:20
This was unintentionally disabled (and incomplete) in the original PR
for pkgimages.

Co-authored-by: Valentin Churavy <v.churavy@gmail.com>
@KristofferC
Copy link
Member

Looks like that fixed most of it indeed.

@oscardssmith
Copy link
Member

Any benchmarks on precompile times with this? I'd be especially interested in the time for OrdinaryDiffEq.

@giordano
Copy link
Contributor

giordano commented Feb 22, 2023

A few days ago I got:
master:

Sysimage built. Summary:
Base ────────  41.622056 seconds 34.7209%
Stdlibs ─────  78.250691 seconds 65.2762%
Total ─────── 119.876211 seconds
[...]
Outputting sysimage file...
Output ────── 227.121960 seconds
    LINK usr/lib/julia/sys.so

PR:

Sysimage built. Summary:
Base ────────  29.183085 seconds 42.5989%
Stdlibs ─────  39.321643 seconds 57.3982%
Total ───────  68.506738 seconds
[...]
Outputting sysimage file...
Output ────── 192.962346 seconds
    LINK usr/lib/julia/sys.so

And for precompile times

Plots Makie
master 71 s 299 s
PR 56 s 278 s

In the depot with Makie installed, size of the compiled/ directory decreased by 3%. Not exactly big improvement, but changes in the right direction.

@vchuravy
Copy link
Member

Hm the sysimage changes are surprising, but nice to see other improvements. We will need to figure out overlap in cache files and then push those down to the common ancestor

@vtjnash
Copy link
Member Author

vtjnash commented Feb 22, 2023

You did make a small, unrelated change to reuse WIP work in the queue. I don't see how that could make it substantially faster though

@IanButterworth
Copy link
Member

We should come up with some tests for this since it can "fail" silently

What about making two packages like

module Foo
# some big generated function
end
module Bar
using Foo
end

and checking that the file size of the .so cache for Foo is larger than Bar?

@vchuravy
Copy link
Member

vchuravy commented Feb 22, 2023

We need to inspect the content, that's the annoying bit

@nanosoldier
Copy link
Collaborator

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

@vchuravy
Copy link
Member

@nanosoldier runtests(["QSimulator", "PopGenCore", "ArgoData", "SeaPearlZoo", "PopGenSims", "GasChem", "AztecDiamonds", "CopEnt", "TrueRandom", "Cambrian", "MultiStateSystems", "BinaryDecisionDiagrams", "TopOptMakie", "GraphMLDatasets", "TrajectoryGamesBase", "Controlz", "RegularizedProblems", "ClimaTimeSteppers", "CalibrationAnalysis", "Integrals", "MRIReco", "IntervalTrees", "LuaCall", "FiniteStateProjection", "LighthouseFlux", "IRKGaussLegendre", "BloqadeODE", "SubSIt"], configuration = (buildflags = ["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],), vs_configuration = (buildflags = ["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"],))

src/debuginfo.cpp Outdated Show resolved Hide resolved
src/jitlayers.cpp Outdated Show resolved Hide resolved
@IanButterworth
Copy link
Member

IanButterworth commented Feb 23, 2023

Some more timing. This is on a 2018 macbook pro.

Master e3d366f

(@v1.10) pkg> st
Status `~/.julia/environments/v1.10/Project.toml`
  [e9467ef8] GLMakie v0.8.2

shell> rm -rf ~/.julia/compiled/v1.10

(@v1.10) pkg> precompile
Precompiling environment...
  201 dependencies successfully precompiled in 669 seconds

shell> du -hs ~/.julia/compiled/v1.10
461M	/Users/ian/.julia/compiled/v1.10

This PR 15d936b

shell> rm -rf ~/.julia/compiled/v1.10

(@v1.10) pkg> precompile
Precompiling environment...
  201 dependencies successfully precompiled in 576 seconds

shell> du -hs ~/.julia/compiled/v1.10
437M	/Users/ian/.julia/compiled/v1.10

This PR 15d936b + LLVM image multithreading #47797 + IanButterworth/Pkg.jl@461bd56 (unrestricting multithreading during Pkg.precompile)

shell> rm -rf ~/.julia/compiled/v1.10

(@v1.10) pkg> precompile
Precompiling environment...
  201 dependencies successfully precompiled in 552 seconds

shell> du -hs ~/.julia/compiled/v1.10
437M	/Users/ian/.julia/compiled/v1.10

Side note, during this all I was running a Google Meet call in chrome and never noticed any video or audio dropout, which is interesting for the last case. FYI @pchintalapudi @staticfloat

So, in conclusion seems like a good improvement, however using everything currently in the works still means TTVFP (Time To Very First Plot) for GLMakie is ~9 minutes for my middle of the road laptop

@vchuravy
Copy link
Member

@nanosoldier runtests(configuration = (buildflags=["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"], julia_flags=["--pkgimages=yes"]), vs_configuration = (buildflags = ["LLVM_ASSERTIONS=1", "FORCE_ASSERTIONS=1"], julia_flags=["--pkgimages=yes"]))

@nanosoldier
Copy link
Collaborator

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

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 23, 2023
@vchuravy
Copy link
Member

Still waiting on the full PkgEval with --pkgimages=true

@DilumAluthge DilumAluthge added needs pkgeval Tests for all registered packages should be run with this change and removed merge me PR is reviewed. Merge when all tests are passing labels Feb 23, 2023
@nanosoldier
Copy link
Collaborator

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

@vchuravy vchuravy removed the needs pkgeval Tests for all registered packages should be run with this change label Feb 25, 2023
@vchuravy vchuravy merged commit c136b7e into master Feb 25, 2023
@vchuravy vchuravy deleted the jn/pkgimg-reuse-external branch February 25, 2023 22:31
fxcoudert pushed a commit to fxcoudert/julia that referenced this pull request Feb 26, 2023
* enable using external code in pkgimages

This was unintentionally disabled (and incomplete) in the original PR
for pkgimages.

Co-authored-by: Valentin Churavy <v.churavy@gmail.com>
vchuravy pushed a commit that referenced this pull request Mar 3, 2023
* enable using external code in pkgimages

This was unintentionally disabled (and incomplete) in the original PR
for pkgimages.

Co-authored-by: Valentin Churavy <v.churavy@gmail.com>
(cherry picked from commit c136b7e)
KristofferC pushed a commit that referenced this pull request Mar 3, 2023
* enable using external code in pkgimages

This was unintentionally disabled (and incomplete) in the original PR
for pkgimages.

Co-authored-by: Valentin Churavy <v.churavy@gmail.com>
(cherry picked from commit c136b7e)
vchuravy added a commit that referenced this pull request Mar 3, 2023
Previously we would double-account for many of these, leading to
occasional chaos. Try to avoid serializing code that does not belong to
this incremental compilation session package.

Refs: #48723
(cherry picked from commit 81f366d)
vchuravy added a commit that referenced this pull request Mar 3, 2023
Previously we would double-account for many of these, leading to
occasional chaos. Try to avoid serializing code that does not belong to
this incremental compilation session package.

Refs: #48723
(cherry picked from commit 81f366d)
vchuravy pushed a commit that referenced this pull request Mar 3, 2023
Previously we would double-account for many of these, leading to
occasional chaos. Try to avoid serializing code that does not belong to
this incremental compilation session package.

Refs: #48723
(cherry picked from commit 81f366d)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants