Region inference: split results from RegionInferenceContext#151688
Region inference: split results from RegionInferenceContext#151688amandasystems wants to merge 4 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
6befd69 to
13a57f6
Compare
This comment has been minimized.
This comment has been minimized.
13a57f6 to
00e8121
Compare
00e8121 to
454f98a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4c01f95 to
174023c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| liveness: &LivenessValues, | ||
| pass_where: PassWhere, | ||
| out: &mut dyn io::Write, | ||
| ) -> io::Result<()> { |
There was a problem hiding this comment.
pls use a consistent function param ordering between the functions here :<
| let (definitions, _has_placeholders) = region_definitions( | ||
| infcx, | ||
| universal_regions, | ||
| &mut constraints.liveness_constraints.clone(), |
There was a problem hiding this comment.
this is... very odd. You give a mutable reference to a clone?
please move the cloned thing into a let binding first
There was a problem hiding this comment.
That's a quick hack workaround, I think we should talk about how to not have to do that, where to put region_definitions(), and whether the liveness constarints stuff is necessary in sync because it's pretty much the same thing.
| outlives_constraints: &'a OutlivesConstraintSet<'tcx>, | ||
| mut w: &mut dyn Write, | ||
| ) -> io::Result<()> { | ||
| dot::render(&RawConstraints { tcx, region_definitions, outlives_constraints }, &mut w) |
There was a problem hiding this comment.
why reference RawConstraints. That type should be Copy
There was a problem hiding this comment.
It's because dot::render() takes its arguments by reference (and it wasn't written by me).
| dot::render(&SccConstraints { tcx, region_definitions, constraint_sccs, nodes_per_scc }, &mut w) | ||
| } | ||
|
|
||
| struct RawConstraints<'a, 'tcx> { |
There was a problem hiding this comment.
make this copy and take it by value
| nodes_per_scc: IndexVec<ConstraintSccIndex, Vec<RegionVid>>, | ||
| region_definitions: &'a IndexVec<RegionVid, RegionDefinition<'tcx>>, | ||
| constraint_sccs: &'a ConstraintSccs, | ||
| } |
| outlives_requirements.as_mut(), | ||
| &mut errors_buffer, | ||
| type_tests, | ||
| ); |
There was a problem hiding this comment.
hmm, I would prefer check_type_tests to stay on the region inference context 🤔
they are a part of region inference to me
There was a problem hiding this comment.
or like, the RegionInferenceCtxt should either not exist at all anymore, or still be used for the region computations 🤔 maybe chat about it in sync
| if let NllRegionVariableOrigin::FreeRegion = origin { | ||
| // Add all nodes in the CFG to liveness constraints for free regions | ||
| liveness_constraints.add_all_points(rvid); | ||
| } |
There was a problem hiding this comment.
this is a functional change? 🤔
There was a problem hiding this comment.
It should not be, this always happens during region inference, I just moved it slightly earlier.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (1ee78dc): comparison URL. Overall result: ❌ regressions - 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 (secondary 2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 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: 479.555s -> 479.91s (0.07%) |
This comment has been minimized.
This comment has been minimized.
|
I think the perf. run needs a comment (there are indications of regressions). Switching to waiting on author to incorporate changes. Feel free to request a review with @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
447cc9d to
685b895
Compare
This comment has been minimized.
This comment has been minimized.
This ensures all of region inference is immutable, and makes every operation that requires region inference to have been executed to run explicitly require the results.
2198237 to
c5fad6f
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. |
|
This fixes the perf regression in one of the benchmarks on my machine (tm) and is even a net improvement in the benchmark I tried. @rustbot ready |
|
I assume you want to check this again. @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.
Region inference: split results from RegionInferenceContext
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (6f40de9): comparison URL. Overall result: ❌ regressions - 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 (secondary -3.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.0%, secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 510.57s -> 510.675s (0.02%) |
View all comments
What?
This PR turns
RegionInferenceContextinto a builder that produces the inferred region values,InferredRegions. The resulting struct implements the public API ofRegionInferenceContextand replaces it inconsumers.rs.RegionInferenceContext::solve()now consumes (moves) the inference context. It is completely private to region inference.Why?
RegionInferenceContexthas become a huge dump for various values people want to access.region_inferitself is a very large file that's difficult to find your way around.RegionInferenceContextnow takes almost all of its fields by referenceKnock-on effects
RegionInferenceContextRegionInferenceContextnow almost exclusively contains references to values, as opposed to owning them. This addresses most offn compute_closure_requirements_modulo_opaquesshouldn't clone all its inputs #146079region_infergains two child modules and becomes a lot less of a behemoth.Detailed overview of changes
region_infer::constraint_searchandregion_infer::universal_regions.handle_placeholdersnow consumes less input and does constraint rewriting via mutable referencesconsumers.rsnow has aInferenceResultsinstead of aRegionInferenceContexteval_{equal, outlives}are moved toInferredRegionsConsumerOptionsis now calledInferredRegionscalculate_borrows_out_of_scope_at_locationof course now takesInferredRegionsLivenessValuesfor free regions is now initialised as live at all points along with the constraint rewriting during placeholder handling, as opposed to during construction ofRegionInferenceContextLeft to cut
Currently
PlaceholderIndicesare still cloned to avoid being moved out ofMirTypeckRegionConstraints. Getting them out ofRegionValuesis complicated since they are needed to iterate over placeholders, and they can't easily be moved out ofMirTypeckRegionConstraints. The obvious solution would be toRcthem, but that's not possible either because they are modified while they reside insideMirTypeckRegionConstraints. By the time they're actually used they're not mutated anymore though, which I've illustraded byFrozen:freeze()'ing them in this version.I didn't want to give
MirTypeckRegionConstraintsthe same treatment as I didRegionInferenceContextto keep this already very very large change as manageable as possible.r? @lcnr
Closes #146079.