Use mut less in dataflow analysis
#148165
Open
+228
−259
Conversation
This file contains hidden or 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
Of the many dataflow analyses, `ConstAnalysis` is the only one that requires the analysis be mutabile when used with `ResultsVisitor`. It's needed because of the `ecx` field -- `ecx.intern_with_temp_alloc` is called during visiting and it takes `&mut self`. This commit changes `ConstAnalysis` to use interior mutability for the `ecx` field. This is a bit annoying for `ConstAnalysis`, but it will allow more immutability in `ResultsVisitor`, as seen in the next commit.
This makes sense -- you wouldn't expect that visiting the results of an analysis would change the analysis itself.
Put `analysis` first, to match `apply_effects_in_range`.
This will let us make `Analysis` arguments in many other places immutable, in the next commit.
rustbot
added
S-waiting-on-author
nnethercote
changed the title
mut less in dataflow analysismut less in dataflow analysis
This comment has been minimized.
This comment has been minimized.
nnethercote
force-pushed
the
less-mut-Analysis
branch
from
564a907 to
e3b54c1
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Oct 27, 2025
Use `mut` less in dataflow analysis
rustbot
added
the
S-waiting-on-perf
hkBst
reviewed
Oct 27, 2025
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (7bd3a30): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 475.144s -> 474.427s (-0.15%) |
rustbot
removed
the
S-waiting-on-perf
The `state: A::Domain` value is the primary things that's modified when performing an analysis. The `Analysis` impl is immutable in every case but one (`MaybeRequiredStorage`) and it now uses interior mutability. As well as changing many `&mut A` arguments to `&A`, this also: - lets `CowMut` be replaced with the simpler `SimpleCow` in `cursor.rs`; - removes the need for the `RefCell` in `Formatter`; - removes the need for `MaybeBorrowedLocals` to impl `Clone`, because it's a unit type and it's now clear that its constructor can be used directly instead of being put into a local variable and cloned.
nnethercote
force-pushed
the
less-mut-Analysis
branch
from
e3b54c1 to
a97cd3b
Compare
|
Some changes occurred to the CTFE machinery Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to constck cc @fee1-dead |
rustbot
added
S-waiting-on-review
`Results` used to contain an `Analysis`, but it was removed in rust-lang#140234. That change made sense because the analysis was mutable but the entry states were immutable and it was good to separate them so the mutability of the different pieces was clear. Now that analyses are immutable there is no need for the separation, lots of analysis+results pairs can be combined, and the names are going back to what they were before: - `Results` -> `EntryStates` - `AnalysisAndResults` -> `Results`
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
&mut Analysisis used a lot:apply_*methods, even though only one visitor needs mutability.I've lost track of the number of times I've thought "why are these
mutagain?" and had to look through the code to remind myself. It's really unexpected, and mostAnalysisinstances are immutable, because thestatevalues are what get mutated.This commit introduces
RefCellin one analysis and one results visitor. This then lets another existingRefCellbe removed, and a ton of&mut Analysisarguments become&Analysis. And thenAnalysisandResultscan be recombined.r? @cjgillot