-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve inference for string operations #44500
Conversation
@@ -1646,7 +1646,7 @@ end | |||
throw_eager_redirection_cycle(key::Union{Char, String}) = | |||
error("Eager redirection cycle detected for key ", repr(key)) | |||
throw_could_not_find_redirected_value(key::Union{Char, String}) = | |||
error("Could not find redirected value ", repl(key)) | |||
error("Could not find redirected value ", repr(key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂
This reduces invalidations when loading packages that define new AbstractString types. There doesn't appear to be any particular need for these methods to be re-specialized for anything besides String.
This is follow-on work to #43939
6a7fd25
to
0012411
Compare
I've been able to eliminate nearly all the invalidations (fewer than 100 left from an initial 1800) with this plus JuliaLang/Pkg.jl#3020 and JuliaIO/Tar.jl#136. |
1323020
to
d1b61ea
Compare
fc98cbe
to
54e486c
Compare
It appears (though there were a few other PRs too) that the combination of this PR and #44628 made |
We may want to consider backporting this. See https://discourse.julialang.org/t/package-load-time-regressions-in-v1-8-beta3/78875?u=tim.holy (I don't know that the invalidation is the cause of the regression, but it's a bad sign). But it's a bit of a scary backport at this point. |
Well, it would be worth testing if backporting this significantly improve things? |
Sure, I guess if you're running PkgEval that pretty much gets rid of the risk. If it runs into trouble we could port the portion of this that is the "second half" of #43939, which if dim memories serve might fix the invalidation of I did confirm that master loads |
This reduces invalidations when loading packages that define new AbstractString types. (cherry picked from commit 515a242)
This PR fixes about 1000 method invalidations for multiple separate packages. The following table lists the number of invalidations on Julia 1.8.2, Julia nightly and this PR: Name | 1.8.2 | 1.9.0-DEV.1506 | This PR --- | --- | --- | --- CSV | 3173 | 5452 | 4560 CategoricalArrays | 3482 | 4713 | 3668 InlineStrings | 1376 | 654 | 654 ProgressLogging | 246 | 2990 | 2049 Generated with: ```julia using Pkg Pkg.activate(; temp=true) Pkg.add(["SnoopCompileCore", "SnoopCompile", "InlineStrings"]) using SnoopCompileCore invs = @Snoopr using InlineStrings using SnoopCompile trees = invalidation_trees(invs) print('\n', trees, '\n') @show length(invs) ``` and running the various Julia versions with `--startup-file=no`. Note that this PR reverts a change introduced by JuliaLang#44500. It changes a `convert(String, X)` back to `string(X)`. Based on this PR reverting an earlier PR and the seemingly random increases and decreases in the table, adding invalidations tests should help avoid regressions (JuliaLang#47022). I'll see whether I can find time to work on that.
People expect to use this (the docs even almost even suggested it at some point), so it is better to make it work as expected (and better than they can emulate) than to criticize their choices. Also fix a few regressions and handling mistakes in setup_stdios: - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect - CmdRedirect did not allocate a ProcessChain, so it wouldd call setup_stdio then call setup_stdios on the result of that, which is strongly discouraged as setup_stdio(s) should only be called once - BufferStream was missing `check_open` calls before writing, and ignored `Base.reseteof` as a possible means of resuming writing after `closewrite` sends a shutdown message. Fixes #39311 Fixes #49234
People expect to use this (the docs even almost even suggested it at some point), so it is better to make it work as expected (and better than they can emulate) than to criticize their choices. Also fix a few regressions and handling mistakes in setup_stdios: - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect - CmdRedirect did not allocate a ProcessChain, so it wouldd call setup_stdio then call setup_stdios on the result of that, which is strongly discouraged as setup_stdio(s) should only be called once - BufferStream was missing `check_open` calls before writing, and ignored `Base.reseteof` as a possible means of resuming writing after `closewrite` sends a shutdown message. Fixes #39311 Fixes #49234 Closes #49233 Closes #46768
People expect to use this (the docs even almost even suggested it at some point), so it is better to make it work as expected (and better than they can emulate) than to criticize their choices. Also fix a few regressions and handling mistakes in setup_stdios: - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect - CmdRedirect did not allocate a ProcessChain, so it wouldd call setup_stdio then call setup_stdios on the result of that, which is strongly discouraged as setup_stdio(s) should only be called once - BufferStream was missing `check_open` calls before writing, and ignored `Base.reseteof` as a possible means of resuming writing after `closewrite` sends a shutdown message. Currently this fix is disabled because Pkg seems like a bit of a disaster with IO mismanagement. - Add `closewrite` to more methods, and document it. Fixes #39311 Fixes #49234 Closes #49233 Closes #46768
People expect to use this (the docs even almost even suggested it at some point), so it is better to make it work as expected (and better than they can emulate) than to criticize their choices. Also fix a few regressions and handling mistakes in setup_stdios: - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect - CmdRedirect did not allocate a ProcessChain, so it wouldd call setup_stdio then call setup_stdios on the result of that, which is strongly discouraged as setup_stdio(s) should only be called once - BufferStream was missing `check_open` calls before writing, and ignored `Base.reseteof` as a possible means of resuming writing after `closewrite` sends a shutdown message. Currently this fix is disabled because Pkg seems like a bit of a disaster with IO mismanagement. - Add `closewrite` to more methods, and document it. Fixes #39311 Fixes #49234 Closes #49233 Closes #46768
People expect to use this (the docs even almost even suggested it at some point), so it is better to make it work as expected (and better than they can emulate) than to criticize their choices. Also fix a few regressions and handling mistakes in setup_stdios: - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect - CmdRedirect did not allocate a ProcessChain, so it wouldd call setup_stdio then call setup_stdios on the result of that, which is strongly discouraged as setup_stdio(s) should only be called once - BufferStream was missing `check_open` calls before writing, and ignored `Base.reseteof` as a possible means of resuming writing after `closewrite` sends a shutdown message. Currently this fix is disabled because Pkg seems like a bit of a disaster with IO mismanagement. - Add `closewrite` to more methods, and document it. Fixes #39311 Fixes #49234 Closes #49233 Closes #46768
People expect to use this (the docs even almost even suggested it at some point), so it is better to make it work as expected (and better than they can emulate) than to criticize their choices. Also fix a few regressions and handling mistakes in setup_stdios: - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect - CmdRedirect did not allocate a ProcessChain, so it wouldd call setup_stdio then call setup_stdios on the result of that, which is strongly discouraged as setup_stdio(s) should only be called once - BufferStream was missing `check_open` calls before writing, and ignored `Base.reseteof` as a possible means of resuming writing after `closewrite` sends a shutdown message. Currently this fix is disabled because Pkg seems like a bit of a disaster with IO mismanagement. - Add `closewrite` to more methods, and document it. Fixes #39311 Fixes #49234 Closes #49233 Closes #46768
People expect to use this (the docs even almost even suggested it at some point), so it is better to make it work as expected (and better than they can emulate) than to criticize their choices. Also fix a few regressions and handling mistakes in setup_stdios: - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect - CmdRedirect did not allocate a ProcessChain, so it would call setup_stdio then call setup_stdios on the result of that, which is strongly discouraged as setup_stdio(s) should only be called once - BufferStream was missing `check_open` calls before writing, and ignored `Base.reseteof` as a possible means of resuming writing after `closewrite` sends a shutdown message. - Add `closewrite` to more methods, and document it. Fixes #39311 Fixes #49234 Fixes #49233 Fixes #46768
Together with JuliaIO/Tar.jl#135, this eliminates nearly
1400>1700 invalidations mostly caused by InlineStrings.jl, and knocks about 300ms off the load time of CSV.jl (CC @quinnj).More benefit still could be achieved from some version of JuliaLang/Pkg.jl#3017 and #44486, but this already helps a lot.