Avoid repeated precompilation when loading from non-relocatable cachefiles #53905
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
staticfloat
approved these changes
Apr 1, 2024
fatteneder
force-pushed
the
fa/avoid-recompilation
branch
from
79d7881
to
d80cec4
Compare
fatteneder
commented
Apr 1, 2024
fatteneder
commented
Apr 1, 2024
giordano
reviewed
Apr 2, 2024
The @depot resolution logic for include() files: 1. If the cache is not relocatable because of an absolute path, we ignore that path for the depot search. Recompilation will be triggered by stale_cachefile() if that absolute path does not exist. 2. If we can't find a depot for a relocatable path, we still replace it with the depot we found from other files. Recompilation will be triggered by stale_cachefile() because the resolved path does not exist. 3. We require that relocatable paths all resolve to the same depot. 4. We explicitly check that all relocatable paths resolve to the same depot. This has two reasons: - We want to scan all source files in order to provide logs for 1. and 2. above. - It is possible that a depot might be missing source files. Assume that we have two depots on DEPOT_PATH, depot_complete and depot_incomplete. If DEPOT_PATH=["depot_complete","depot_incomplete"] then no recompilation shall happen, because depot_complete will be picked. If DEPOT_PATH=["depot_incomplete","depot_complete"] we trigger recompilation and hopefully a meaningful error about missing files is thrown. If we were to just select the first depot we find, then whether recompilation happens would depend on whether the first relocatable file resolves to depot_complete or depot_incomplete.
fatteneder
force-pushed
the
fa/avoid-recompilation
branch
from
f769fdf
to
db64a5d
Compare
KristofferC
pushed a commit
that referenced
this pull request
Apr 9, 2024
…files (#53905) Fixes #53859 (comment), which was actually fixed before in #52346, but #52750 undid that fix. This PR does not directly address #53859, because I can not reproduce it atm. --- The `@depot` resolution logic for `include()` files is adjusted as follows: 1. (new behavior) If the cache is not relocatable because of an absolute path, we ignore that path for the depot search. Recompilation will be triggered by `stale_cachefile()` if that absolute path does not exist. Previously this caused any `@depot` tags to be not resolved and so trigger recompilation. 2. (new behavior) If we can't find a depot for a relocatable path, we still replace it with the depot we found from other files. Recompilation will be triggered by `stale_cachefile()` because the resolved path does not exist. 3. (this behavior is kept) We require that relocatable paths all resolve to the same depot. 4. (new behavior) We no longer use the first matching depot for replacement, but instead we explicitly check that all resolve to the same depot. This has two reasons: - We want to scan all source files anyways in order to provide logs for 1. and 2. above, so the check is free. - It is possible that a depot might be missing source files. Assume that we have two depots on `DEPOT_PATH`, `depot_complete` and `depot_incomplete`. If `DEPOT_PATH=["depot_complete","depot_incomplete"]` then no recompilation shall happen, because `depot_complete` will be picked. If `DEPOT_PATH=["depot_incomplete","depot_complete"]` we trigger recompilation and hopefully a meaningful error about missing files is thrown. If we were to just select the first depot we find, then whether recompilation happens would depend on whether the first relocatable file resolves to `depot_complete` or `depot_incomplete`. (cherry picked from commit d8d3842)
KristofferC
added a commit
that referenced
this pull request
Apr 9, 2024
Backported PRs: - [x] #53757 <!-- Add an IndexStyle example to the diagind docstring --> - [x] #53809 <!-- Add missing GC_POP() in emit_cfunction --> - [x] #53789 <!-- also check that UUID of project is non-null when treating it as a package --> - [x] #53805 <!-- precompilepkgs: simplify custom config printing if only one --> - [x] #53822 <!-- Bump libuv --> - [x] #53837 <!-- update MPFR to 4.2.1 --> - [x] #53862 <!-- precompilepkgs: fix error reporting --> - [x] #53774 <!-- Remove some duplicates from emitted compilation traces --> - [ ] #53696 <!-- add invokelatest to on_done callback in bracketed paste --> - [x] #53383 <!-- Add `_unsetindex!` methods for `SubArray`s and `CartesianIndex`es --> - [x] #53475 <!-- Fix boundscheck in unsetindex for SubArrays --> - [x] #53888 - [x] #53870 <!-- Revert change to checksum for llvm-julia --> - [x] #53906 <!-- Add `Base.isrelocatable(pkg)` --> - [x] #53833 <!-- Profile: make heap snapshots viewable in vscode viewer --> - [x] #53961 <!-- `LazyString` in `LinearAlgebra.checksquare` error message --> - [x] #53962 <!-- Use StringMemory instead of StringVector where possible --> - [x] #53825 <!-- profile: doc: update the `Allocs.@profile` doc string --> - [x] #53975 <!-- `LazyString` in `DimensionMismatch` error messages in broadcasting --> - [x] #53905 <!-- Avoid repeated precompilation when loading from non-relocatable cachefiles --> - [x] #53896 <!-- Make reshape and view on Memory produce Arrays and delete wrap --> - [x] #53991 <!-- Test and fix non-int-length bug in `view(::Memory, ::Union{UnitRange, Base.OneTo})` -->
KristofferC
removed
the
backport 1.11
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #53859 (comment), which was actually fixed before in #52346, but #52750 undid that fix.
This PR does not directly address #53859, because I can not reproduce it atm.
The
@depotresolution logic forinclude()files is adjusted as follows:stale_cachefile()if that absolute path does not exist. Previously this caused any@depottags to be not resolved and so trigger recompilation.stale_cachefile()because the resolved path does not exist.DEPOT_PATH,depot_completeanddepot_incomplete.If
DEPOT_PATH=["depot_complete","depot_incomplete"]then no recompilation shall happen, becausedepot_completewill be picked.If
DEPOT_PATH=["depot_incomplete","depot_complete"]we trigger recompilation and hopefully a meaningful error about missing files is thrown. If we were to just select the first depot we find, then whether recompilation happens would depend on whether the first relocatable file resolves todepot_completeordepot_incomplete.