-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Rollup of 3 pull requests #148324
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
Rollup of 3 pull requests #148324
Conversation
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.
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.
`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`
…jgillot Use `mut` less in dataflow analysis `&mut Analysis` is used a lot: - In results visitors, even though only one visitor needs mutability. - In all the `apply_*` methods, even though only one visitor needs mutability. I've lost track of the number of times I've thought "why are these `mut` again?" and had to look through the code to remind myself. It's really unexpected, and most `Analysis` instances are immutable, because the `state` values are what get mutated. This commit introduces `RefCell` in one analysis and one results visitor. This then lets another existing `RefCell` be removed, and a ton of `&mut Analysis` arguments become `&Analysis`. And then `Analysis` and `Results` can be recombined. r? `@cjgillot`
…dead Fix deferred cast checks using the wrong body for determining constness re-introduces rust-lang#103683 it was lost in rust-lang@4fec845 deferred checks possibly have more such issues, needs investigation. r? `@fee1-dead`
bootstrap: Extract parts of `bootstrap::core::builder` into a `cli_paths` module One of the things that makes bootstrap's CLI path handling hard to work with is the fact that it's in the middle of a ~2000 line file full of all sorts of other things. And the primary code sequence is in an unhelpfully-named `StepDescription::run` function. This PR therefore pulls some key chunks of code out into a `cli_paths` submodule. This should be a purely non-functional change.
|
Rollup of everything. @bors r+ rollup=never p=5 |
|
☀️ Test successful - checks-actions |
|
📌 Perf builds for each rolled up PR:
previous master: 23c7bad921 In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 23c7bad (parent) -> 51f5892 (this PR) Test differencesShow 10 test diffsStage 1
Stage 2
Additionally, 4 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 51f5892019f8fb07864647d46c4eb577d3b0719f --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (51f5892): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression 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 -1.7%, secondary 2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 473.971s -> 474.501s (0.11%) |
Successful merges:
mutless in dataflow analysis #148165 (Usemutless in dataflow analysis)bootstrap::core::builderinto acli_pathsmodule #148317 (bootstrap: Extract parts ofbootstrap::core::builderinto acli_pathsmodule)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup