Replace closures_captures and upvar_capture with closure_min_captures #82951
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit a54eefb with merge b08e4d30cf5eda2d10489d7a35a19886954075dd... |
|
☀️ Try build successful - checks-actions |
|
Queued b08e4d30cf5eda2d10489d7a35a19886954075dd with parent 3a5d45f, future comparison URL. |
|
Finished benchmarking try commit (b08e4d30cf5eda2d10489d7a35a19886954075dd): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
Some degree of the improvement can be accounted for the fact that there is less information in typechk results, and therefore there is less to hash when building the incr cache. I'm not sure where the increase in incr count is coming from, it is mostly <1%. Maybe it's because how the data is now organized which now requires us to access two data structrues instead of just one and also we create an iterator so there are added checks for that? |
| @@ -693,7 +676,7 @@ impl<'tcx> TypeckResults<'tcx> { | |||
| impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckResults<'tcx> { | |||
| fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) { | |||
| let ty::TypeckResults { | |||
| hir_owner, | |||
| hir_owner: _, | |||
There was a problem hiding this comment.
Hmm, this used to be hashed as part of hashing the upvar_capture_map (we asserted it was equal to something we hashed). I'm not sure if it needs to be hashed, but it's also not obvious that it doesn't. I'd rather add a line like this:
hcx.local_def_path_hash(hir_owner);(Unless you know a reason not to, of course!)
There was a problem hiding this comment.
I'd assume it still does get hashed, upvar_capture_map got replaced by closure_min_capture map, which contains places, which represent their start(PlaceBase) using an UpvarId. These upvar ids are the same as those in the upvar_capture_map, they are now associated with other information like projections, types etc.
|
r=me, but I'd like to see the hashing change @bors delegate+ |
|
✌️ @jenniferwills can now approve this pull request |
make changes to liveness to use closure_min_captures use different span borrow check uses new structures rename to CapturedPlace stop using upvar_capture in regionck remove the bridge cleanup from rebase + remove the upvar_capture reference from mutability_errors.rs remove line from livenes test make our unused var checking more consistent update tests adding more warnings to the tests move is_ancestor_or_same_capture to rustc_middle/ty update names to reflect the closures add FIXME check that all captures are immutable borrows before returning add surrounding if statement like the original move var out of the loop and rename Co-authored-by: Logan Mosier <logmosier@gmail.com> Co-authored-by: Roxane Fruytier <roxane.fruytier@hotmail.com>
arora-aman
force-pushed
the
wr-mir-replace-methods2
branch
from
a54eefb
to
88db752
Compare
|
@bors r=nikomatsakis |
|
📌 Commit 88db752 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
@jenniferwills @nikomatsakis After merging another performance run was done on this PR which shows regressions larger than 1%, meaning we might want to look into it. The regression is coming in incremental builds in the |
|
@rylev hmm, offhand no -- is this doing more encoding than previously? I'd actually expect less encoding work, since we removed some fields I think. |
|
Only thing that has changed since the first perf run is that now we explicitly hash the |
Removed all uses of closures_captures and upvar_capture and refactored code to work with closure_min_captures. This also involved removing functions that were no longer needed like the bridge.
Closes rust-lang/project-rfc-2229#18
r? @nikomatsakis