Borrowck: Simplify SCC annotation computation, placeholder rewriting #151696
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
amandasystems
force-pushed
the
scc-annotations-update
branch
from
3989564 to
56873b6
Compare
This comment has been minimized.
This comment has been minimized.
|
merge conflict :3 |
WITH UPSTREAM! I must have accidentally based this on an old fork or something. |
This simplifies the `PlaceholderReachability` `enum` by replacing the case when no placeholders were reached with a standard `Option::None`. It also rewrites the API for `scc::Annotations` to be update-mut rather than a more Functional programming style. This showed some slight performance impact in early tests of the PR and definitely makes the implementation simpler.
amandasystems
force-pushed
the
scc-annotations-update
branch
from
56873b6 to
4bdccab
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@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.
Borrowck: Simplify SCC annotation computation, placeholder rewriting
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f2cb709): comparison URL. Overall result: ❌✅ regressions and 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 -2.3%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 473.018s -> 472.115s (-0.19%) |
rustbot
removed
the
S-waiting-on-perf
|
Uuuh ok so I wasn’t excepting that kind of memory use improvement and it might be a fluke but I’LL TAKE IT. I guess it’s possible an option may sometimes be cheaper than a 3-value enum, who knows compilers are weird 🤷♀️ |
|
@bors r+ |
rust-bors
bot
added
S-waiting-on-bors
|
No idea how serious to take memory use changes here, I treat this PR as mostly perf neutral. The code is nicer though :3 |
They look like the usual variance/noise, yes. So this is safe to rollup. @bors rollup=maybe |
…uwer Rollup of 12 pull requests Successful merges: - #150491 (resolve: Mark items under exported ambiguous imports as exported) - #150720 (Do not suggest `derive` if there is already an impl) - #150968 (compiler-builtins: Remove the no-f16-f128 feature) - #151493 ([RFC] rustc_parse: improve the error diagnostic for "missing let in let chain") - #151660 (Bump `std`'s `backtrace`'s `rustc-demangle`) - #151696 (Borrowck: Simplify SCC annotation computation, placeholder rewriting) - #151704 (Implement `set_output_kind` for Emscripten linker) - #151706 (Remove Fuchsia from target OS list in unix.rs for sleep) - #151769 (fix undefined behavior in VecDeque::splice) - #151779 (stdarch subtree update) - #151449 ([rustdoc] Add regression test for #151411) - #151773 (clean up checks for constant promotion of integer division/remainder operations)
Rollup merge of #151696 - amandasystems:scc-annotations-update, r=lcnr Borrowck: Simplify SCC annotation computation, placeholder rewriting This change backports some changes from the now abandoned #142623. Notably, it simplifies the `PlaceholderReachability` `enum` by replacing the case when no placeholders were reached with a standard `Option::None`. It also rewrites the API for `scc::Annotations` to be update-mut rather than a more Functional programming style. This showed some slight performance impact in early tests of the PR and definitely makes the implementation simpler. This probably wants a perf run just for good measure. r? @lcnr
…uwer Rollup of 12 pull requests Successful merges: - rust-lang/rust#150491 (resolve: Mark items under exported ambiguous imports as exported) - rust-lang/rust#150720 (Do not suggest `derive` if there is already an impl) - rust-lang/rust#150968 (compiler-builtins: Remove the no-f16-f128 feature) - rust-lang/rust#151493 ([RFC] rustc_parse: improve the error diagnostic for "missing let in let chain") - rust-lang/rust#151660 (Bump `std`'s `backtrace`'s `rustc-demangle`) - rust-lang/rust#151696 (Borrowck: Simplify SCC annotation computation, placeholder rewriting) - rust-lang/rust#151704 (Implement `set_output_kind` for Emscripten linker) - rust-lang/rust#151706 (Remove Fuchsia from target OS list in unix.rs for sleep) - rust-lang/rust#151769 (fix undefined behavior in VecDeque::splice) - rust-lang/rust#151779 (stdarch subtree update) - rust-lang/rust#151449 ([rustdoc] Add regression test for rust-lang/rust#151411) - rust-lang/rust#151773 (clean up checks for constant promotion of integer division/remainder operations)
6 participants
This change backports some changes from the now abandoned #142623.
Notably, it simplifies the
PlaceholderReachabilityenumby replacing the case when no placeholders were reached with a standardOption::None.It also rewrites the API for
scc::Annotationsto be update-mut rather than a more Functional programming style. This showed some slight performance impact in early tests of the PR and definitely makes the implementation simpler.This probably wants a perf run just for good measure.
r? @lcnr