Improve inference for string operations #44500
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vtjnash
reviewed
Mar 7, 2022
vtjnash
reviewed
Mar 7, 2022
vtjnash
reviewed
Mar 7, 2022
vtjnash
reviewed
Mar 7, 2022
| @@ -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)) | |||
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
timholy
force-pushed
the
teh/inlinestrings_invalidations
branch
from
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. |
timholy
force-pushed
the
teh/inlinestrings_invalidations
branch
from
1323020
to
d1b61ea
Compare
timholy
commented
Mar 11, 2022
timholy
force-pushed
the
teh/inlinestrings_invalidations
branch
from
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 |
KristofferC
pushed a commit
that referenced
this pull request
Apr 19, 2022
This reduces invalidations when loading packages that define new AbstractString types. (cherry picked from commit 515a242)
67 tasks
rikhuijzer
added a commit
to rikhuijzer/julia
that referenced
this pull request
Oct 4, 2022
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.
vtjnash
added a commit
that referenced
this pull request
Dec 8, 2023
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
vtjnash
added a commit
that referenced
this pull request
Dec 8, 2023
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
vtjnash
added a commit
that referenced
this pull request
Dec 8, 2023
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
vtjnash
added a commit
that referenced
this pull request
Dec 8, 2023
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
vtjnash
added a commit
that referenced
this pull request
Dec 9, 2023
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
vtjnash
added a commit
that referenced
this pull request
Dec 11, 2023
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
vtjnash
added a commit
that referenced
this pull request
Dec 13, 2023
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.