timholy / SnoopCompile.jl

Provide insights about latency (TTFX) for Julia packages
https://timholy.github.io/SnoopCompile.jl/dev/
Other
309 stars 48 forks source link

WIP - Reentrant concurrent snoopi_deep profiles. #309

Closed NHDaly closed 1 year ago

NHDaly commented 1 year ago

NOTE: It turns out this PR isn't mergeable as-is, since it depends on assumptions in Julia that aren't valid. Namely, we need https://github.com/JuliaLang/julia/pull/47258 and https://github.com/JuliaLang/julia/pull/47260 to avoid data races, but it turns out those don't make sense against julia master/1.9, because of recent changes to the inference lock.

At RAI, we have backported those two PRs against 1.8.2, and we're going to keep hacking on this branch to pull it into our internal build. We will follow-up with actual changes to julia 1.9 and SnoopCompile to try to make this work for real.


Following option 3 from this doc: https://docs.google.com/document/d/1JO43w3V1Cwtb2CxWf4sYfAB68xbkTrCMr973iaJOFwY/edit#

This is a WIP PR to support invoking @snoopi_deep concurrently and in parallel from multiple Tasks. (It currently requires https://github.com/JuliaLang/julia/pull/47258 to avoid data races, and https://github.com/JuliaLang/julia/pull/47260 to allow reentrant inference timing.)

The idea is basically to share the global julia type inference profile buffer, rather than clear it at the start of every profile, keeping track of the start and stop index for each invocation of snoopi_deep. Then we also clear the beginning of the profile buffer whenever the oldest invocation ends, to ensure we aren't leaking memory.

NHDaly commented 1 year ago

Note that of course one must be careful when interpreting the results of concurrently running profiles, since inference caused by invocation B will also show up in the profile for invocation A. But this is true of all of julia's profiling and timing techniques right now, so it's just something the developer must be aware of. (Compare it with @time from multiple tasks reporting allocations caused by other tasks, or reporting compilation time from other tasks.)

codecov[bot] commented 1 year ago

Codecov Report

Base: 85.47% // Head: 17.39% // Decreases project coverage by -68.08% :warning:

Coverage data is based on head (f86ce5b) compared to base (ad792af). Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #309 +/- ## =========================================== - Coverage 85.47% 17.39% -68.09% =========================================== Files 17 16 -1 Lines 2162 1903 -259 =========================================== - Hits 1848 331 -1517 - Misses 314 1572 +1258 ``` | [Impacted Files](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy) | Coverage Δ | | |---|---|---| | [SnoopCompileCore/src/snoopi\_deep.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-U25vb3BDb21waWxlQ29yZS9zcmMvc25vb3BpX2RlZXAuamw=) | `0.00% <0.00%> (-92.00%)` | :arrow_down: | | [SnoopCompileCore/src/snoopl.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-U25vb3BDb21waWxlQ29yZS9zcmMvc25vb3BsLmps) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [src/parcel\_snoopl.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-c3JjL3BhcmNlbF9zbm9vcGwuamw=) | `0.00% <0.00%> (-90.00%)` | :arrow_down: | | [src/parcel\_snoopi\_deep.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-c3JjL3BhcmNlbF9zbm9vcGlfZGVlcC5qbA==) | `0.00% <0.00%> (-89.91%)` | :arrow_down: | | [src/invalidation\_and\_inference.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-c3JjL2ludmFsaWRhdGlvbl9hbmRfaW5mZXJlbmNlLmps) | `0.00% <0.00%> (-89.66%)` | :arrow_down: | | [src/invalidations.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-c3JjL2ludmFsaWRhdGlvbnMuamw=) | `0.00% <0.00%> (-83.92%)` | :arrow_down: | | [src/deep\_demos.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-c3JjL2RlZXBfZGVtb3Muamw=) | `0.00% <0.00%> (-58.98%)` | :arrow_down: | | [src/SnoopCompile.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-c3JjL1Nub29wQ29tcGlsZS5qbA==) | `66.66% <0.00%> (-33.34%)` | :arrow_down: | | [SnoopPrecompile/src/SnoopPrecompile.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-U25vb3BQcmVjb21waWxlL3NyYy9Tbm9vcFByZWNvbXBpbGUuamw=) | `56.25% <0.00%> (-31.25%)` | :arrow_down: | | ... and [7 more](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/309/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

NHDaly commented 1 year ago

Update: it turns out that https://github.com/JuliaLang/julia/pull/47258 and https://github.com/JuliaLang/julia/pull/47260 are no longer valid, now that we're shrinking the inference lock (which is 🎉 in general though of course!).

So this PR will have to wait, because we'll probably need to rethink the inference profiler entirely, given that it currently assumes single-threaded, locked access to the inference profile buffer. :/

NHDaly commented 1 year ago

CC: @sacha0 for review plz

NHDaly commented 1 year ago

Thanks again for the review, Sacha. I'm closing this for now, since we ended up going a different approach, via https://github.com/JuliaLang/julia/pull/47615 and https://github.com/timholy/SnoopCompile.jl/pull/313.