Consider captured regions for opaque type region liveness.#156027
Consider captured regions for opaque type region liveness.#156027jackh726 wants to merge 1 commit intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
Until I can look at this next week, @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
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%) |
rustbot
removed
the
S-waiting-on-perf
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.
| desc { "listing captured lifetimes for opaque `{}`", tcx.def_path_str(def_id) } | ||
| } | ||
|
|
||
| query opaque_live_args(def_id: DefId) -> &'tcx ty::EarlyBinder<'tcx, Vec<ty::GenericArg<'tcx>>> { |
There was a problem hiding this comment.
We should ensure the query is documented, and if not, add the doc comment from the fn here as well.
| /// 2. If there are *any* outlives bounds, Then we find any args that outlive those bounds. | ||
| /// 3. If there are *no* outlives bounds, then we return all non-bivariant args, since they could potentially be relevant. | ||
| #[tracing::instrument(level = "debug", skip(tcx), ret)] | ||
| pub(crate) fn opaque_live_args<'tcx>( |
| // 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.
| } | ||
| rustc_hir::OpaqueTyOrigin::TyAlias { parent, .. } => (parent, IndexSet::default()), | ||
| }; | ||
|
|
There was a problem hiding this comment.
The section below could also use comments.
| /// Given a known `param_env` and a set of well formed types, set up an | ||
| /// `InferCtxt`, call the passed function (to e.g. set up region constraints | ||
| /// to be tested), then resolve region and return errors | ||
| fn test_region_obligations<'tcx>( |
| // 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).
5 participants
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