-
Notifications
You must be signed in to change notification settings - Fork 13.7k
-Znext-solver
: support non-defining uses in closures
#145925
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
Conversation
@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.
support non-defining uses in closures
This comment has been minimized.
This comment has been minimized.
f736683
to
e9dcabd
Compare
eaa44ba
to
4d7c068
Compare
This comment has been minimized.
This comment has been minimized.
4d7c068
to
22d33e3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
378c073
to
4406446
Compare
-Znext-solver
support non-defining uses in closures (追記ここまで)
-Znext-solver
support non-defining uses in closures (削除ここまで)-Znext-solver
: support non-defining uses in closures (追記ここまで)
Finished benchmarking commit (f55097d): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @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 -1.0%, secondary -2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.8%)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: 467.222s -> 467.244s (0.00%) |
tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs
Outdated
Show resolved
Hide resolved
4406446
to
417a251
Compare
This comment has been minimized.
This comment has been minimized.
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.
I agree that this shouldn't affect stable. Member constraints in parent bodies being affected by region requirements from defining uses in child bodies would require an external region being influenced by member constraints which is not possible.
I think it's unfortunate to not properly handle the cyclic dependency of child bodies and parent bodies both having uses of opaques that may depend on region constraints from eachother. I think it's fine to maybe do something about that in the future as part of a larger cleanup of the member constraint algorithm.
r=me once u fix that test
compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs
Outdated
Show resolved
Hide resolved
417a251
to
ef9653e
Compare
This comment has been minimized.
This comment has been minimized.
ef9653e
to
60171d5
Compare
since boxy has given an r+: I love it but the last commit could use a more descriptive message than whaddup gamers, before landing :3.
This comment has been minimized.
This comment has been minimized.
60171d5
to
1c3a6ee
Compare
support non-defining uses in closures
1c3a6ee
to
b8160e9
Compare
@bors r=BoxyUwU rollup=never
☀️ Test successful - checks-actions
Approved by: BoxyUwU
Pushing 75ee9ff to master...
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 7aef4be (parent) -> 75ee9ff (this PR)
Test differences
Show 250 test diffs
Stage 1
[ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs
: pass -> [missing] (J0)[ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs#current
: [missing] -> pass (J0)[ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs#next
: [missing] -> pass (J0)[ui] tests/ui/impl-trait/non-defining-uses/recursive-call.rs#current
: [missing] -> pass (J0)[ui] tests/ui/impl-trait/non-defining-uses/recursive-call.rs#next
: [missing] -> pass (J0)
Stage 2
[ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs
: pass -> [missing] (J1)[ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs#current
: [missing] -> pass (J1)[ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs#next
: [missing] -> pass (J1)[ui] tests/ui/impl-trait/non-defining-uses/recursive-call.rs#current
: [missing] -> pass (J1)[ui] tests/ui/impl-trait/non-defining-uses/recursive-call.rs#next
: [missing] -> pass (J1)
Additionally, 240 doctest diffs were found. These are ignored, as they are noisy.
Job group index
- J0: x86_64-gnu-llvm-19-3, x86_64-gnu-llvm-20-3
- J1: aarch64-apple, aarch64-gnu, aarch64-gnu-llvm-19-1, aarch64-msvc-1, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, test-various, x86_64-gnu, x86_64-gnu-debug, x86_64-gnu-llvm-19, x86_64-gnu-llvm-19-2, x86_64-gnu-llvm-20-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
Test dashboard
Run
cargo run --manifest-path src/ci/citool/Cargo.toml -- \ test-dashboard 75ee9ffd5ed3649c0a09493057adaa8feebb2035 --output-dir test-dashboard
And then open test-dashboard/index.html
in your browser to see an overview of all executed tests.
Job duration changes
- pr-check-1: 1335.7s -> 1728.3s (29.4%)
- x86_64-rust-for-linux: 2644.5s -> 3170.8s (19.9%)
- aarch64-apple: 7320.6s -> 6075.6s (-17.0%)
- aarch64-gnu-llvm-19-1: 3235.0s -> 3714.9s (14.8%)
- x86_64-gnu-llvm-19: 2381.1s -> 2729.6s (14.6%)
- dist-x86_64-apple: 7384.2s -> 6325.8s (-14.3%)
- dist-arm-linux-gnueabi: 5505.5s -> 4717.7s (-14.3%)
- test-various: 4444.8s -> 5029.7s (13.2%)
- x86_64-gnu-debug: 7319.5s -> 8240.0s (12.6%)
- i686-gnu-nopt-1: 7291.5s -> 8192.4s (12.4%)
How to interpret the job duration changes?
Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.
Finished benchmarking commit (75ee9ff): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@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 0.0%, secondary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.2%)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: 469.026s -> 467.688s (-0.29%) |
Do you think it's worth it to investigate the regressions?
I don't think so, it's a small consistent regression and we are doing a bit more work
Ok 👍
@rustbot label: +perf-regression-triaged
Uh oh!
There was an error while loading. Please reload this page.
Cleaned up version of #139587, finishing the implementation of rust-lang/types-team#129. This does not affect stable. The reasoning for why this is the case is subtle however.
What does it do
We split
do_mir_borrowck
intoborrowck_collect_region_constraints
andborrowck_check_region_constraints
, whereborrowck_collect_region_constraints
returns an enormousCollectRegionConstraintsResult
struct which contains all the relevant data to actually handle opaque type uses and to check the region constraints later on.query mir_borrowck
now simply callsBorrowCheckRootCtxt::do_mir_borrowck
which starts by iterating over all nested bodies of the current function - visiting nested bodies before their parents - and computing theirCollectRegionConstraintsResult
.After we've collected all constraints it's time to actually compute the concrete types for the opaques defined by this function. With this PR we now compute the concrete types of opaques for each body before using them to check the non-defining uses of any of them.
After we've computed the concrete types by using all bodies, we use
apply_computed_concrete_opaque_types
for each body to constrain non-defining uses, before finally finishing withborrowck_check_region_constraints
. We always visit nested bodies before their parents when doing this.ClosureRegionRequirements
As we only call
borrowck_collect_region_constraints
for nested bodies before type checking the parent, we can't simply use the finalClosureRegionRequirements
of the nested body during MIR type check. We instead track that we need to apply these requirements indeferred_closure_requirements
.We now manually apply the final closure requirements to each body after handling opaque types.
This works, except that we may need the region constraints of nested bodies to successfully define an opaque type in the parent. This is handled by using a new
fn compute_closure_requirements_modulo_opaques
which duplicates region checking - while ignoring any errors - before we've added the constraints fromapply_computed_concrete_opaque_types
. This is necessary for a lot of async tests, as pretty much the entire function is inside of an async block while the opaque type gets defined in the parent.As an performance optimization we only use
fn compute_closure_requirements_modulo_opaques
in case the nested body actually depends on any opaque types. Otherwise we eagerly callborrowck_check_region_constraints
and apply the final closure region requirements right away.Impact on stable code
Handling the opaque type uses in the parent function now only uses the closure requirements modulo opaques, while it previously also considered member constraints from nested bodies.
External
regions are never valid choice regions. Also, member constraints will never constrain a member region if it is required to be outlived by an external region, as that fails the upper-bound check.rust/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs
Lines 90 to 96 in 564ee21
Member constraints therefore never add constraints for external regions :>
r? @BoxyUwU