[WIP] traits/select: use global vs per-infcx caches more uniformly. #69294

eddyb · 2020-02-19T17:59:36Z

~~Note: this is based on an older master to avoid perf interference before #67953 (comment) is resolved.~~ EDIT: sadly not workable due #69294 (comment).

So far this PR only has the first couple steps towards making the decision of which cache ("global" vs "per-InferCtxt") to use for trait evaluation/selection, only once (at the time of checking the cache).

The goal here is to actually make per-InferCtxt caches not track DepNodes, and maybe even enforce that once SelectionContext::in_task is entered, the InferCtxt is effectively unused.

My assumption is that if you need inference variables in your cache key (i.e. ParamEnv and/or PolyTraitPredicate) to ever end up doing anything "non-global", and you can't get there from a "global cache key" (which would still use DepNodes and in_task etc.).

Perhaps another path forward is to redirect "global cache keys" to a query, but I'm not sure how that would handle cycles (badly?), and it feels like stepping on Chalk's toes.

r? @nikomatsakis cc @rust-lang/wg-traits @Zoxc @michaelwoerister

eddyb · 2020-02-19T18:03:46Z

@Mark-Simulacrum Ugh, I was hoping to do this on top of 58b8343, before all of the recent rustc::{infer,traits} changes, but bors seems to want to merge against current master.

eddyb · 2020-02-19T18:16:21Z

@bors try @rust-timer queue

rust-timer · 2020-02-19T18:16:23Z

Awaiting bors try build completion

bors · 2020-02-19T18:16:37Z

⌛ Trying commit 6a51d66 with merge a540ddc...

@nikomatsakis

[WIP] traits/select: use global vs per-infcx caches more uniformly. ~~*Note: this is based on an older `master` to avoid perf interference before #67953 (comment) is resolved.*~~ **EDIT**: sadly not workable due #69294 (comment). So far this PR only has the first couple steps towards making the decision of which cache ("global" vs "per-`InferCtxt`") to use for trait evaluation/selection, only once (at the time of checking the cache). The goal here is to actually make per-`InferCtxt` caches not track `DepNode`s, and maybe even enforce that once `SelectionContext::in_task` is entered, the `InferCtxt` is effectively unused. My assumption is that if you *need* inference variables in your cache key (i.e. `ParamEnv` and/or `PolyTraitPredicate`) to ever end up doing anything "non-global", and you can't get there from a "global cache key" (which would still use `DepNode`s and `in_task` etc.). Perhaps another path forward is to redirect "global cache keys" to a query, but I'm not sure how that would handle cycles (badly?), and it feels like stepping on Chalk's toes. r? @nikomatsakis cc @rust-lang/wg-traits @Zoxc @michaelwoerister

Mark-Simulacrum · 2020-02-19T19:10:48Z

Ugh, I was hoping to do this on top of 58b8343, before all of the recent rustc::{infer,traits} changes, but bors seems to want to merge against current master.

AFAIK, if you really want this you can add revert commits to this branch for master's commits since then and then compare with an older base commit (vs. using the link perf auto-generates).

bors · 2020-02-19T21:13:25Z

☀️ Try build successful - checks-azure
Build commit: a540ddc (a540ddca4b5ca4451bed53f0b13b03b5db95c9ec)

rust-timer · 2020-02-19T21:13:27Z

Queued a540ddc with parent 7710ae0, future comparison URL.

eddyb · 2020-02-19T21:43:59Z

@Mark-Simulacrum I was shocked at how far down I had to go in git log to find the PRs I wanted to base myself just before, I don't think I could seriously consider reverting all of them, heh.

Mark-Simulacrum · 2020-02-19T21:58:23Z

In theory git revert -m1 $(git log <base PR bors commit> --author=bors --format %H) should work, but I haven't tested :)

eddyb · 2020-02-19T22:09:20Z

It's more that I would find that excessive and it would make a mess of the PR.

eddyb · 2020-02-20T09:08:43Z

Surprised this is already a win, this is good news (results are available, idk why the bot didn't post another comment).

eddyb · 2020-02-24T17:22:15Z

src/librustc_infer/traits/select.rs

-            Ok(Some(SelectionCandidate::ParamCandidate(trait_ref))) => {
-                !trait_ref.skip_binder().input_types().any(|t| t.walk().any(|t_| t_.is_ty_infer()))
-            }
+            Ok(Some(SelectionCandidate::ParamCandidate(trait_ref))) => !trait_ref.needs_infer(),


This is a fix for #55258 (comment), I should make a separate commit but I didn't want to lose track of this.

nikomatsakis · 2020-02-25T19:12:13Z

BTW I'm planning to review this today or tomorrow

nikomatsakis

This all makes a lot of sense to me thus far. Sorry for being a bit slow.

nikomatsakis · 2020-03-02T18:32:17Z

src/librustc_infer/traits/select.rs

+            &self.infcx.selection_cache
+        };
+
+        cache.hashmap.borrow().get(&param_env.and(*trait_ref)).map(|v| v.get(tcx))


to be clear, this commit is just a "logical cleanup", right?

nikomatsakis · 2020-03-02T18:33:13Z

src/librustc_infer/traits/select.rs

-                }
-            }
+        // HACK(eddyb) never cache overflow (this check used to be global-only).
+        if let Err(Overflow) = candidate {


with the exception of this tiny logic change, I guess

I can't really think why we would want to cache overflow locally anyhow, that seems like a bug...

Pfft, that's nothing, here's something more fun: I don't think we ever use the local caches except for the intercrate mode, because we're doing freshening all the time.

So we could put intercrate in Reveal or w/e then replace freshening with canonicalization, and have only global caching (either that or I'm seriously misunderstanding the implementation).

bors · 2020-03-14T20:03:41Z

☔ The latest upstream changes (presumably #69076) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2020-03-30T13:56:08Z

@eddyb is this PR considered "eligible to merge" or is it blocked on something?

eddyb · 2020-04-06T03:34:05Z

@nikomatsakis This is waiting for M1 from w3f/General-Grants-Program#261 (i.e. self-profiling integration for the trait system), so that we can measure the impact of this. I'll likely open a PR for that soon.

Dylan-DPC-zz · 2020-09-11T00:33:17Z

@eddyb the grants PR has been merged, is there anything else this is blocked on?

nikomatsakis · 2020-11-19T14:39:35Z

I'm going to close this PR for inactivity. @eddyb re-open if you want when you think it's ready to go forward

rust-highfive assigned nikomatsakis Feb 19, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2020

This comment has been minimized.

Sign in to view

eddyb force-pushed the trait-cache-streamline branch from 2a24386 to 6a51d66 Compare February 19, 2020 18:15

eddyb added 3 commits February 24, 2020 19:15

traits/select: simplify "has inference vars" check.

b71da57

traits/select: use global vs per-infcx caches more uniformly.

8b9e4ba

traits/select: pass the whole cache key to can_use_global_caches.

2bc133c

eddyb force-pushed the trait-cache-streamline branch from 6a51d66 to 2bc133c Compare February 24, 2020 17:21

eddyb commented Feb 24, 2020

View reviewed changes

eddyb mentioned this pull request Feb 25, 2020

Fix Rustdoc ICE when checking blanket impls #55258

Merged

nikomatsakis approved these changes Mar 2, 2020

View reviewed changes

Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2020

eddyb added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2020

eddyb marked this pull request as draft April 10, 2020 20:33

nikomatsakis closed this Nov 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] traits/select: use global vs per-infcx caches more uniformly. #69294

[WIP] traits/select: use global vs per-infcx caches more uniformly. #69294

eddyb commented Feb 19, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Feb 19, 2020

eddyb commented Feb 19, 2020

rust-timer commented Feb 19, 2020

bors commented Feb 19, 2020

Mark-Simulacrum commented Feb 19, 2020

bors commented Feb 19, 2020

rust-timer commented Feb 19, 2020

eddyb commented Feb 19, 2020

Mark-Simulacrum commented Feb 19, 2020 •

edited

Loading

eddyb commented Feb 19, 2020

eddyb commented Feb 20, 2020

eddyb Feb 24, 2020

nikomatsakis commented Feb 25, 2020

nikomatsakis left a comment

nikomatsakis Mar 2, 2020

nikomatsakis Mar 2, 2020

nikomatsakis Mar 2, 2020

eddyb Mar 2, 2020 •

edited

Loading

bors commented Mar 14, 2020

nikomatsakis commented Mar 30, 2020

eddyb commented Apr 6, 2020

Dylan-DPC-zz commented Sep 11, 2020 •

edited

Loading

nikomatsakis commented Nov 19, 2020

[WIP] traits/select: use global vs per-infcx caches more uniformly. #69294

[WIP] traits/select: use global vs per-infcx caches more uniformly. #69294

Conversation

eddyb commented Feb 19, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Feb 19, 2020

eddyb commented Feb 19, 2020

rust-timer commented Feb 19, 2020

bors commented Feb 19, 2020

Mark-Simulacrum commented Feb 19, 2020

bors commented Feb 19, 2020

rust-timer commented Feb 19, 2020

eddyb commented Feb 19, 2020

Mark-Simulacrum commented Feb 19, 2020 • edited Loading

eddyb commented Feb 19, 2020

eddyb commented Feb 20, 2020

eddyb Feb 24, 2020

Choose a reason for hiding this comment

nikomatsakis commented Feb 25, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Mar 2, 2020

Choose a reason for hiding this comment

nikomatsakis Mar 2, 2020

Choose a reason for hiding this comment

nikomatsakis Mar 2, 2020

Choose a reason for hiding this comment

eddyb Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 14, 2020

nikomatsakis commented Mar 30, 2020

eddyb commented Apr 6, 2020

Dylan-DPC-zz commented Sep 11, 2020 • edited Loading

nikomatsakis commented Nov 19, 2020

eddyb commented Feb 19, 2020 •

edited

Loading

Mark-Simulacrum commented Feb 19, 2020 •

edited

Loading

eddyb Mar 2, 2020 •

edited

Loading

Dylan-DPC-zz commented Sep 11, 2020 •

edited

Loading