Adapt SnoopPrecompile & snoopi_deep to work for new julia API. #313
Conversation
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>
NHDaly
force-pushed
the
nhd-pv-SnoopPrecompile-new-jl-api
branch
from
d40b6eb to
40a9f6c
Compare
Codecov ReportBase: 83.56% // Head: 83.22% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #313 +/- ##
==========================================
- Coverage 83.56% 83.22% -0.34%
==========================================
Files 17 17
Lines 2178 2188 +10
==========================================
+ Hits 1820 1821 +1
- Misses 358 367 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Create a dummy ROOT node, and return the tree with that node.
| function finish_snoopi_deep() | ||
| # Construct a dummy Timing for ROOT(), for backwards compatibility with the old API. | ||
| root = Timing( | ||
| InferenceFrameInfo(ROOTmi, 0x0, Any[], Any[Core.Const(ROOT)], 1), | ||
| 0x0) | ||
| root.children = Core.Compiler.Timings.clear_and_fetch_timings() | ||
|
|
||
| return InferenceTimingNode(root) |
There was a problem hiding this comment.
@timholy: In JuliaLang/julia#47615, we changed the API to return a vector of inference trees, rather than a single inference tree with the dummy "ROOT" node.
We think this is a more sensical API, since there really wasn't a single tree of type inference, there were many, and it also made it easier to get the thread safety correct.
However, of course this breaks the old API. So for now, I'm proposing we just do this, and shove it back into a dummy ROOT node to make a single tree 😅
Then we can investigate changes to the API in the future if we want to clean things up to all operate on vectors of trees, or just leave it like this forever. :)
Does that sound good to you?
There was a problem hiding this comment.
I think this LGTM. 👍 The tests pass on our local branch of JuliaLang/julia#47615, and on a release build, so I think this is good to go.
Thanks @vilterp. 👍
|
There are some other test failures here, but they seem very much unrelated. If anyone (@timholy?) can comment on it, that'd be appreciated, otherwise I plan to merge this PR next week. :) |
NHDaly
changed the title
|
I'd certain approve merging this if it's needed for Julia 1.10! Sorry for my lack of response. |
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.