Allow threadsafe access to buffer of type inference profiling trees#47615
Allow threadsafe access to buffer of type inference profiling trees#47615vilterp wants to merge 2 commits intoJuliaLang:masterfrom
Conversation
|
Mysterious error in the tests: Can't find the actual error message. Anyone know what this is? |
|
Looks like that failing test is coming from here: I guess the julia tests must pull in the Pkg.jl tests. And those tests are cloning and testing the EnglishText.jl package 😅 It seems probably unrelated, although it's a little weird that those same tests keep failing on multiple commits.. 🤔 |
NHDaly
added
backport 1.8
|
I tried pulling down Pretty wild that SnoopCompile is part of julia build tests like this. But also very cool! |
Starting in JuliaLang/julia#47615, we are changing the API for snoopi_deep, to allow for thread safe snooping in one thread while compiling in another. This commit changes SnoopPrecompile to work with the new API if it's available.
Starting in JuliaLang/julia#47615, we are changing the API for snoopi_deep, to allow for thread safe snooping in one thread while compiling in another. This commit changes SnoopPrecompile to work with the new API if it's available. Co-Authored-By: Pete Vilter <pete.vilter@gmail.com>
vchuravy
changed the title
@vchuravy or @pchintalapudi: do you have any advice on how best to mark the old functions as deprecated? I was thinking about possibly moving the implementations to Does that seem like the right approach to you? |
|
Eh, at that point Core.Compiler.Timings should be closed. So I think that won't work. Arguably it's a compiler internals so subject to change and doesn't need deprecation. |
|
K, thanks, that's helpful. 👍 |
|
Maybe we can still log a deprecation warning via |
|
or maybe that's worse than no log at all, since you can't turn it off via the no-deprecation-warnings commandline flag. 🤷 no strong opinions here |
|
👍 let us know when this is good for final review |
vchuravy
force-pushed
the
pv-compiler-timings-lock
branch
from
689a5e0 to
5696689
Compare
vilterp
force-pushed
the
pv-compiler-timings-lock
branch
from
8f02963 to
9e6be01
Compare
Co-authored-by: Nathan Daly <NHDaly@gmail.com>
Starting in 1.9, julia does not hold the codegen lock for all of typeinference! This means that we cannot use a single shared scratch-space for profiling type inference. This PR changes the scratch space to use Task Local Storage, so that we build a separate inference profile tree per Task doing inference, and then report them to the global results buffer at the end.
NHDaly
force-pushed
the
pv-compiler-timings-lock
branch
from
2f7972b to
1009210
Compare
| if t.storage === nothing | ||
| t.storage = IdDict() | ||
| end | ||
| return (t.storage)::IdDict{Any,Any} |
There was a problem hiding this comment.
@vchuravy / @pchintalapudi / @kpamnany: I finally got around to testing this, and indeed it doesn't work. :(
It turns out that Base.IdDict and Core.Compiler.IdDict are not the same thing. 😢
Does anyone know why that is? Why doesn't Base just reexport the IdDict from Core.Compiler?
I.e. here:
Line 165 in 80aeebe
Why doesn't it just reexport the IdDict from Core.Compiler?
There was a problem hiding this comment.
BUT anyway, the error we encounter is that we're not allowed to set the Task's .storage field to a Core.Compiler.IdDict; it's expected to be a Base.IdDict.
So is there just no way to use task-local storage from Core.Compiler?
There was a problem hiding this comment.
Possibly a more robust solution here, which might also last longer if we parallelize inference, would be to store this on the AbstractInterpreter or InferenceState objects? Does anyone know if that's plausible? We essentially want some object that lives for the lifetime of the inference invocation, and is local to this invocation, where we can store the stack of timers.
There was a problem hiding this comment.
Or we'd have to thread it through every function call, but that seems like possibly a nonstarter.
There was a problem hiding this comment.
So is there just no way to use task-local storage from Core.Compiler?
Nobody needed this before it seems :)
Does anyone know why that is? Why doesn't Base just reexport the IdDict from Core.Compiler?
Likely to isolate the compiler code from Base.
There was a problem hiding this comment.
You can do:
if isdefined(Core, :Main)
Core.Main.Base.get_task_tls(current_task())
end
to get around the bootstrapping issue.
There was a problem hiding this comment.
It is because the compiler is forbidden from accessing TLS. Since we hijack the current task to run codegen, it could corrupt the running task if it does anything to interact with it.
There was a problem hiding this comment.
(instead, you could perhaps replace the TLS on entry with one from Core.Compiler, and restore it on return)
|
This still has I think we shouldn't hold up 1.9 on this. Should I remove the backport label? CC: @vchuravy It means that users will need to be careful not to run |
… upstream: JuliaLang/julia#47615 was never merged, so this feature cannot be used yet anyway. :( And the compat bounds are currently too strict, and are causing issues in RelationalAI's build
… upstream: JuliaLang/julia#47615 was never merged, so this feature cannot be used yet anyway. :( And the compat bounds are currently too strict, and are causing issues in RelationalAI's build
… upstream: JuliaLang/julia#47615 was never merged, so this feature cannot be used yet anyway. :( And the compat bounds are currently too strict, and are causing issues in RelationalAI's build
… upstream. (#25) * Remove type inference profiling, since thread safety was never merged upstream: JuliaLang/julia#47615 was never merged, so this feature cannot be used yet anyway. :( And the compat bounds are currently too strict, and are causing issues in RelationalAI's build * Bump to version v1.1.0
Backported PRs: - [x] #47782 <!-- Generalize Bool parse method to AbstractString --> - [x] #48634 <!-- Remove unused "deps" mechanism in internal sorting keywords [NFC] --> - [x] #49931 <!-- Lock finalizers' lists at exit --> - [x] #50064 <!-- Fix numbered prompt with input only with comment --> - [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized --> - [x] #50516 <!-- Fix visibility of assert on GCC12/13 --> - [x] #50635 <!-- `versioninfo()`: include build info and unofficial warning --> - [x] #49915 <!-- Revert "Remove number / vector (#44358)" --> - [x] #50781 <!-- fix `bit_map!` with aliasing --> - [x] #50845 <!-- fix #50438, use default pool for at-threads --> - [x] #49031 <!-- Update inference.md --> - [x] #50289 <!-- Initialize prev_nold and nold in gc_reset_page --> - [x] #50559 <!-- Expand kwcall lowering positional default check to vararg --> - [x] #49582 <!-- Update HISTORY.md for `DelimitedFiles` --> - [x] #50341 <!-- invokelatest docs should say not exported before 1.9 --> - [x] #50525 <!-- only check that values are finite in `generic_lufact` when `check=true` --> - [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some cases --> - [x] #50523 <!-- Avoid generic call in most cases for getproperty --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50164 <!-- codegen: handle dead code with unsafe_store of FCA pointers --> - [x] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [x] #50871 <!-- macOS: Don't inspect dead threadtls during exception handling. --> Need manual backport: - [ ] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [ ] #50591 <!-- build: fix various makefile bugs --> Non-merged PRs with backport label: - [ ] #50842 <!-- Avoid race conditions with recursive rm --> - [ ] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #49716 <!-- Update varinfo() docstring signature --> - [ ] #49713 <!-- prevent REPL from erroring in numbered mode in some situations --> - [ ] #49573 <!-- Implement jl_cpu_pause on PPC64 --> - [ ] #48726 <!-- fix macro expansion of property destructuring --> - [ ] #48642 <!-- Use gc alloc instead of alloc typed in lowering --> - [ ] #48183 <!-- Don't use pkgimage for package if any includes fall in tracked path for coverage or alloc tracking --> - [ ] #48050 <!-- improve `--heap-size-hint` arg handling --> - [ ] #47615 <!-- Allow threadsafe access to buffer of type inference profiling trees -->
|
Okay, so this PR was opened in November 2022, and it was approved by @jpsamaroo in January 2023. However, since that approval, I see some comments from Julian suggesting that the PR doesn't actually work. Additionally, there has been no activity on this PR for two years+. @jpsamaroo @vilterp What is the status of this PR? Are y'all still working on it? |
DilumAluthge
added
status: waiting for PR reviewer
and removed
backport 1.9
|
Nope. And the entire type inference infrastructure has changed in 1.11 anyway. Thanks, we'll close this. |
6 participants
This PR allows accessing the results of type inference profiling (via
@snoopi_deep) from a separate background thread.Note that type inference on multiple threads while snooping is enabled is still not threadsafe as of this PR, since starting in 1.9 type inference can happen in parallel on multiple threads. We will fix this in a follow-up PR.
In the meantime, this is a step in the right direction, and fixes thread safety for the specific case of one profiling thread and one inference thread, and this PR could be backported to 1.8.*.
TODO:
print deprecation warning (printlnand@warnare undefined in the compiler)