A grab bag of latency reductions #35714
Conversation
|
I also stole a couple of commits from the https://github.com/JuliaLang/julia/tree/kc/stale_precompile branch. (Thanks @KristofferC!) |
|
Build failure is just for docs (I didn't try that locally). I might wait to see if anybody has any insight about the logging issues first before I decide to tackle this further. Sure would be nice to see this in 1.5, of course, so if needed I can back that last commit out and do surgery on Revise. |
This is a companion PR to JuliaLang/julia#35714 and accounts for a subset of the gains reported there. The more specific implementations of `==` and `hash` avoid a `collect`; that branch prevents many `collect`-induced invalidations via the `iteratorsize` trick, but this change is probably a good idea anyway. The more important and subtle one is the one in TOML._print. This is used by a large number of methods in Pkg, so it's important to prevent it from being invalidated. Do to nospecialize markers, this method gets inferred for partially- specified types, and the `a` is typically just `AbstractDict`. However, `Iterators.Pairs` is also an AbstractDict, and the return type of `keys` cannot be inferred from the parameters of `AbstractDict{K,V}` because the `itr` field of a pair uses a later parameter. This short-circuits the whole problem by "hand-inlining" the result of `invoke(keys, Tuple{AbstractDict})` at the call site, i.e., just creating a `KeySet` directly.
|
Forgot there was an important companion I needed to push: JuliaLang/Pkg.jl#1806. That |
|
OK, so the difference in method sorting mentioned above seems to be that My guess is that this occurs because |
| @@ -502,7 +513,7 @@ with_logger(logger) do | |||
| end | |||
| ``` | |||
| """ | |||
| with_logger(f::Function, logger::AbstractLogger) = with_logstate(f, LogState(logger)) | |||
| with_logger(@nospecialize(f::Function), logger::AbstractLogger) = with_logstate(f, LogState(logger)) | |||
There was a problem hiding this comment.
I would not expect this to make a difference; it doesn't affect inference and we will already avoid specializing this function on f.
There was a problem hiding this comment.
Yeah, it doesn't seem to change anything.
| """ | ||
| copyto!(dest, src) | ||
|
|
||
| function copyto!(dest::AbstractArray{T1,N}, src::AbstractArray{T2,N}) where {T1,T2,N} |
There was a problem hiding this comment.
Is this method really not needed?
There was a problem hiding this comment.
I'm not actually sure, for now this is just a guess. Have not yet run tests or benchmarks, to me that will be the proof of the pudding.
There was a problem hiding this comment.
Here's a good one: on master,
julia> a = [1 2; 3 4]
2×2 Array{Int64,2}:
1 2
3 4
julia> b = [3, 1, 4, 2]
4-element Array{Int64,1}:
3
1
4
2
julia> copyto!(view(a, b), a)
4-element view(::Array{Int64,1}, [3, 1, 4, 2]) with eltype Int64:
1
3
1
1vs
julia> copyto!(view(a, b), copy(a))
4-element view(::Array{Int64,1}, [3, 1, 4, 2]) with eltype Int64:
1
3
2
4I'm hoping to fix it in this PR, or will report it if not.
This is a companion PR to JuliaLang/julia#35714 and accounts for a subset of the gains reported there. The more specific implementations of `==` and `hash` avoid a `collect`; that branch prevents many `collect`-induced invalidations via the `iteratorsize` trick, but this change is probably a good idea anyway. The more important and subtle one is the one in TOML._print. This is used by a large number of methods in Pkg, so it's important to prevent it from being invalidated. Do to nospecialize markers, this method gets inferred for partially- specified types, and the `a` is typically just `AbstractDict`. However, `Iterators.Pairs` is also an AbstractDict, and the return type of `keys` cannot be inferred from the parameters of `AbstractDict{K,V}` because the `itr` field of a pair uses a later parameter. This short-circuits the whole problem by "hand-inlining" the result of `invoke(keys, Tuple{AbstractDict})` at the call site, i.e., just creating a `KeySet` directly.
timholy
force-pushed
the
teh/less_invalidation
branch
from
c341326 to
7a1bf8c
Compare
|
OK, I seem to have discovered an exit on the logging framework: the combination of I've not yet run timing comparisons on anything meaningful to see what this actually adds up to in practice; it's entirely possible that the improvement will be modest. (It may be complicated because it will also depend on what other packages get loaded.) It's also quite possible this will cause dreadful runtime regressions. If this passes tests, let's run our benchmarks and find out---to me it seems that the experiment is well worth doing. |
|
OK, this is cool. |
|
I've been vaguely wondering about how backedges interact with the logging system for a while. I have noticed that installing a new logger causes a lot more recompilation than you might expect or want, but I sure didn't realize that It's also known that setting the global logger doesn't really work well with the world age - see #33865 . That would suggest, perhaps, we should have an So anyway, I'm cautiously positive about just breaking some backedges here. The logging system needs some other refactoring to fix (runtime) performance issues as well so it's probably a fair bit slower than it could be already. |
When the eltype and/or dimensionality is unspecified, there were 2 or 3 methods that might apply here (see `code_typed(copyto!, (Array, Array))`). Consequently partially-specified codegen tends to be poor for this operation. This harmonizes our implementations and eliminates eltype and dimensionality as a consideration for the dispatch. By default, it dispatches on `IndexStyle(src)` after a call to `unalias`, which makes sense because copying an array can change its preferred `IndexStyle`. It also fixes an aliasing bug.
CodeTracking.jl (a foundation for Revise & JuliaInterpreter) defines additional `PkgId` methods; leaving this untyped leads to invalidation of the entire code-loading path.
This aims to reduce disruption when adding methods to `convert` and `copymutable`.
timholy
force-pushed
the
teh/less_invalidation
branch
from
7a1bf8c to
f83a84c
Compare
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Well, that's rather unremarkable (which is what I was hoping for). @JeffBezanson, let me know if there's anything objectionable here before merging. |
Well I doubt we have any logging performance tests ;-) But anyway I feel like this is probably a good usability tradeoff for now. In the future we can do some proper performance benchmarking and optimization of the logging system. |
This is a companion PR to JuliaLang/julia#35714 and accounts for a subset of the gains reported there. The more specific implementations of `==` and `hash` avoid a `collect`; that branch prevents many `collect`-induced invalidations via the `iteratorsize` trick, but this change is probably a good idea anyway. The more important and subtle one is the one in TOML._print. This is used by a large number of methods in Pkg, so it's important to prevent it from being invalidated. Do to nospecialize markers, this method gets inferred for partially- specified types, and the `a` is typically just `AbstractDict`. However, `Iterators.Pairs` is also an AbstractDict, and the return type of `keys` cannot be inferred from the parameters of `AbstractDict{K,V}` because the `itr` field of a pair uses a later parameter. This short-circuits the whole problem by "hand-inlining" the result of `invoke(keys, Tuple{AbstractDict})` at the call site, i.e., just creating a `KeySet` directly. (cherry picked from commit d30adaf)
Yesterday I learned about
JL_DEBUG_METHOD_INVALIDATIONand have spent some time analyzing the sources of invalidation-related latencies from loading Revise. Having Revise be slow to start up is not exactly what we want for a package designed to reduce "practical" latency, so this seemed like an important case to optimize. But I expect some of these changes will have more widespread impacts.This is organized in a series of independent commits; for details, it's probably best to look at the commit message for each one. The bottom line is that this reduces the number of lines from
julia --startup-file=no -e "using Revise" &> /tmp/invalidated.log, when compiled withJL_DEBUG_METHOD_INVALIDATION = 1, by more than 200 lines.That may sound good (and it is!), but there's an even more awesome prize waiting (EDIT: fixed below): figure out how to prevent massive invalidation due to the logging framework (CC @c42f). Revise defines its own custom logger, and this seems to invalidate basically every method throughout Base and packages that uses a
@debug,@info,@warn, or@errormacro. That's really expensive, and I'm contemplating removing all the logging from Revise just to prevent this from happening, but it would be a sad loss.I made a guess that it was
catch_exception, see final commit, but this was not effective. The culprit really does appear to beshouldlog, and I'm surprised becauseReviseLoggerends up last on its method table so I don't understand why this invalidates so much.ReviseLoggerends up first on the method table formin_enabled_level, so maybe that's the real problem? (I honestly don't understand why it sorts that way, it would seem that having equally-specific methods insert at the end of the method table makes the most sense and that seems to be what happens most of the time.)With these changes and if I disable Revise's custom logger, I go from about 1300 lines of invalidation-output to around 100. That has a very noticeable positive effect on "snappiness" when using Revise. It sure would be nice to get there!