Consider captured regions for opaque type region liveness.#156027
Conversation
|
Until I can look at this next week, @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.
Consider captured regions for opaque type region liveness.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (97f0332): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary -2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.2%, secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 481.713s -> 483.106s (0.29%) |
There was a problem hiding this comment.
I also started down this direction so some of this makes sense to me. We stopped because it felt fragile and test coverage was, and still is, weak. So with that caveat this generally feels fine to me, but again, I don't know enough about this area to say what problems there could be.
This direction felt less nice than what lcnr described in the t-types meeting where we described the issue, so I'll defer to them.
| // Thinking about it, I was originally a bit concerned about something like `'a: 'static`, and | ||
| // whether or not we need to mark `'a` as live. I don't think *today* we do, since I think regions | ||
| // that outlive `'static` are special enough, but I *could* imagine some world where we need to be | ||
| // more careful about this. Given I can't find a test that goes wrong, I'm going to leave in this | ||
| // optimization. |
There was a problem hiding this comment.
I was also a bit worried, and we should expand test coverage here. I'm not confident enough in any of this to trust the two tests we have.
| // Unfortunately, we have to use a new `InferCtxt` each call, because | ||
| // region constraints get added and solved there and we need to test each | ||
| // call individually. |
There was a problem hiding this comment.
This is also unfortunate, combined with the fact that it's being called a quadratic number of times (thankfully the N should be small in most cases).
This comment has been minimized.
This comment has been minimized.
0718cad to
ca97579
Compare
|
@bors try |
This comment has been minimized.
This comment has been minimized.
Consider captured regions for opaque type region liveness.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot p=2 We want to get this in so that we can do a nightly preview of Polonius |
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
| /// (and so is `T`, since `T: 'static` implies `T: 'a`) | ||
| /// - `bar` outlives `'static`, so we know that no args are potentially live and we can return an empty set | ||
| /// - `baz` has no outlives bound, so return `None` and let the caller decide what to do | ||
| query live_args_for_alias_from_outlives_bounds(kind: ty::AliasTyKind<'tcx>) -> &'tcx Option<ty::EarlyBinder<'tcx, Vec<ty::GenericArg<'tcx>>>> { |
There was a problem hiding this comment.
There should no difference between AliasTyKind and DefId wrt what information they give you. Both are just a DefId, where AliasTyKind also has some information about DefKind in there.
Reconstructing an AliasTy from a DefId is intentionally vaguely annoying 🤔 even though I'd expect that the query system is significantly better at dealing with raw DefIds I guess it's whatever whether to use AliasTy or DefId here
| let parent_param_env = tcx.param_env(parent_def_id); | ||
| tracing::debug!(?parent_param_env); | ||
|
|
||
| // Map the outlives regions to the parent regions |
There was a problem hiding this comment.
I mean concretely for this loop 🤔 this is a lot of nothing which feels unrelated to the actual semantics of this function?
Like, we take self_identity_args and give back a parent_outlives_regions. I do not understand why this is necessary or what exactly it even does. What does parent_outlives_regions represent, why do we need to do this etc?
bf2a68f to
203af9a
Compare
This comment has been minimized.
This comment has been minimized.
203af9a to
1912e4f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…arams, use tcx.assumed_wf_types, and test polonius_alpha in gat-outlives-implied test
NLLs introduced liveness to allow invalidations on dead loans, and its regions are sets of CFG points. The points where the regions are live, the regions they outlive (in a flow-insensitive manner) and vice-versa, and so on are used to compute the CFG points that represent regions. A loan is considered live at a point if the region where it was introduced contains this point (modulo kills, unimportant here). To support flow-sensitivity, polonius regions are sets of loans, and loan liveness needs to track the outlives relationships between regions more precisely: a loan is considered live if it is contained in a live region. Polonius thus needs the accurate liveness of regions to check if an invalidation is indeed an error. As an NLL superset, if a loan is contained in a dead region and is invalidated, polonius will accept that as well. If the region is unexpectedly dead like in the #153215 examples, this is of course a soundness issue, and this PR fixes the current inaccuracy in liveness for some regions related to aliases. The approach makes sense to me, and we've talked about it with jack and niko, and despite the complexity I think this is an improvement we need. There may be ways to simplify in the near future, a part of it is caused by the mapping and our current encoding of opaque types, etc. lcnr has told jack that we can land this if I'm also happy with it, and I am. It does get us closer to enabling polonius alpha on nightly. If there are more improvements to do after this handful of review rounds, I think it's fine to do them in follow-ups. Let's do this 🚀, @bors r+ |
Consider captured regions for opaque type region liveness. Fixes rust-lang#153215 For opaques, when we're calculating liveness for opaques, we want to consider any captured lifetimes that can outlive the opaque type, which is more than just the outlives bounds. r? lqd
…uwer Rollup of 24 pull requests Successful merges: - #150946 (intrinsics: Add a fallback for non-const libm float functions) - #158510 (Enable `static_position_independent_executables` on all gnu and musl targets) - #158541 (Move `std::io::Write` to `core::io`) - #158899 (Fix `unaligned_volatile_store` by removing `MemFlags::UNALIGNED`) - #156027 (Consider captured regions for opaque type region liveness.) - #156370 (Reject linked dylib EII default overrides) - #156508 (Infer all anonymous lifetimes in assoc consts as `'static`) - #157561 (rustdoc: do not include extra stuff in span) - #158617 (allow mGCA const arguments to fall back to anon consts) - #158645 (Fix splat ICEs and ban it in closures) - #158859 (Improve `-Zls` diagnostic message on `.rs` files) - #158988 (Redo `TokenStreamIter`) - #158347 (Improve generic parameters handling for #[diagnostic::on_const]) - #158384 (Allow BackwardIncompatibleDropHint in polonius legacy) - #158722 (delegation: do not always inherit `ConstArgHasType` predicates) - #158739 (view-types: HIR lowering) - #158877 (borrowck: Keep returned `path` from `best_blame_constraint()` consistent) - #158883 (tests: fix enum-match.rs to handle LLVM 23) - #158886 (Add documentation for the `no_std` attribute) - #158940 (Implement feature `char_to_u32`) - #158951 (Merge three `MaxUniverse`s into one) - #158961 (Reapply "LLVM 23: Run AssignGUIDPass in some places") - #158995 (Use REST API in linkchecker script) - #158996 ([compiler] Implement `PartialOrd` via `Ord` for `Span` and newtype_indexes)
Consider captured regions for opaque type region liveness. Fixes rust-lang#153215 For opaques, when we're calculating liveness for opaques, we want to consider any captured lifetimes that can outlive the opaque type, which is more than just the outlives bounds. r? lqd
Rollup of 24 pull requests Successful merges: - #150946 (intrinsics: Add a fallback for non-const libm float functions) - #158541 (Move `std::io::Write` to `core::io`) - #156027 (Consider captured regions for opaque type region liveness.) - #156370 (Reject linked dylib EII default overrides) - #156508 (Infer all anonymous lifetimes in assoc consts as `'static`) - #157561 (rustdoc: do not include extra stuff in span) - #158617 (allow mGCA const arguments to fall back to anon consts) - #158645 (Fix splat ICEs and ban it in closures) - #158859 (Improve `-Zls` diagnostic message on `.rs` files) - #158988 (Redo `TokenStreamIter`) - #158347 (Improve generic parameters handling for #[diagnostic::on_const]) - #158384 (Allow BackwardIncompatibleDropHint in polonius legacy) - #158722 (delegation: do not always inherit `ConstArgHasType` predicates) - #158739 (view-types: HIR lowering) - #158877 (borrowck: Keep returned `path` from `best_blame_constraint()` consistent) - #158883 (tests: fix enum-match.rs to handle LLVM 23) - #158886 (Add documentation for the `no_std` attribute) - #158940 (Implement feature `char_to_u32`) - #158951 (Merge three `MaxUniverse`s into one) - #158960 (Fix bootstrap submodule path prefix matching) - #158961 (Reapply "LLVM 23: Run AssignGUIDPass in some places") - #158995 (Use REST API in linkchecker script) - #158996 ([compiler] Implement `PartialOrd` via `Ord` for `Span` and newtype_indexes) - #159036 (bootstrap: expand '@argfile' arguments to rustc shim)
Rollup merge of #156027 - jackh726:opaque-liveness, r=lqd Consider captured regions for opaque type region liveness. Fixes #153215 For opaques, when we're calculating liveness for opaques, we want to consider any captured lifetimes that can outlive the opaque type, which is more than just the outlives bounds. r? lqd
|
@rust-timer build 9b082ed |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9b082ed): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in 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.3%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.2%, secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 490.768s -> 490.144s (-0.13%) |
|
@rustbot label: +perf-regression-triaged |
View all comments
Fixes #153215
For opaques, when we're calculating liveness for opaques, we want to consider any captured lifetimes that can outlive the opaque type, which is more than just the outlives bounds.
r? lqd