Skip to content

Micro-optimize InferCtxt::replace_bound_vars_with_placeholders by avoiding unnecessary allocation #96021

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Micro-optimize InferCtxt::replace_bound_vars_with_placeholders by avoiding unnecessary allocation #96021

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions compiler/rustc_infer/src/infer/higher_ranked/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// (i.e., if there are no placeholders).
let next_universe = self.universe().next_universe();

let mut region_replaced = false;

let fld_r = |br: ty::BoundRegion| {
region_replaced = true;
self.tcx.mk_region(ty::RePlaceholder(ty::PlaceholderRegion {
universe: next_universe,
name: br.kind,
Expand All @@ -100,11 +103,12 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
})
};

let (result, map) = self.tcx.replace_bound_vars(binder, fld_r, fld_t, fld_c);
let result =
self.tcx.replace_escaping_bound_vars(binder.skip_binder(), fld_r, fld_t, fld_c);

// If there were higher-ranked regions to replace, then actually create
// the next universe (this avoids needlessly creating universes).
if !map.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait...is this wrong?

I think this should be taking into account types and consts too...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we see bound types in practice? Like other than in canonical queries, which I assume we liberate elsewhere? (maybe not)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't :) At least not much today. Which is likely why this isn't doesn't fail everywhere. But in the future, we'll probably see more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the wrongness pre-existing? This change looks ok to me from a mechanical refactoring point of view, but I don't know much about this code in general.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks preexisting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do you have any concern with this PR being merged?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! But someone (probably me) should file an issue so this doesn't get forgotten.

if region_replaced {
let n_u = self.create_next_universe();
assert_eq!(n_u, next_universe);
}
Expand All @@ -113,8 +117,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
"replace_bound_vars_with_placeholders(\
next_universe={:?}, \
result={:?}, \
map={:?})",
next_universe, result, map,
region_replaced={:?})",
next_universe, result, region_replaced,
);

result
Expand Down