Query-ify Instance::resolve #67797

Aaron1011 · 2020-01-02T03:10:42Z

Split off from #65989

Instance::resolve is now a wrapper for a new resolve_instance query.
This greatly improves performance on several benchmarks

rust-highfive · 2020-01-02T03:10:46Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

Centril · 2020-01-02T03:15:06Z

r? @nikomatsakis cc @Zoxc @eddyb

@bors try @rust-timer queue

rust-timer · 2020-01-02T03:15:08Z

Awaiting bors try build completion

bors · 2020-01-02T03:15:19Z

⌛ Trying commit d1eb2de827f187de057cacbcc4c62edff2b9c45a with merge 609a6dac721d35c178f8b8147e885b190fc01e35...

bors · 2020-01-02T05:53:44Z

☀️ Try build successful - checks-azure
Build commit: 609a6dac721d35c178f8b8147e885b190fc01e35 (609a6dac721d35c178f8b8147e885b190fc01e35)

rust-timer · 2020-01-02T05:53:46Z

Queued 609a6dac721d35c178f8b8147e885b190fc01e35 with parent 0ec3706, future comparison URL.

rust-timer · 2020-01-02T07:35:10Z

Finished benchmarking try commit 609a6dac721d35c178f8b8147e885b190fc01e35, comparison URL.

Aaron1011 · 2020-01-02T22:25:29Z

The style-servo benchmark seems to be completely missing from the perf run

Mark-Simulacrum · 2020-01-02T22:29:12Z

style-servo is currently broken (on master, too): "error: truncating cast: the value 4294967292 requires 32 bits but the target type is only 16 bits" (amongst others).

nikomatsakis

Thoughts?

nikomatsakis · 2020-01-03T17:29:10Z

src/librustc/ty/instance.rs

-        };
-        debug!("resolve(def_id={:?}, substs={:?}) = {:?}", def_id, substs, result);
-        result
+        tcx.resolve_instance((param_env, def_id, substs))


Hmm, this seems like a place where canonicalization would be appropriate. We don't expect inference variables, but we do use this (I believe) without having fully erased regions, right? In that case, canonicalizing might eliminate duplicate work.

I agree. However, I think #67717 is already doing canonicalization changes related to (among other things) Instance::resolve. My queryification of Instance::resolve is just a copy-paste of the existing method body, so it should be hopefully be straightforward to apply one PR on top of the other (depending on which gets merged first).

Heh, I just came here from reviewing #67717 to point out that it is making changes to Instance::resolve that would require canonicalization.

@nikomatsakis: Would you like me to add canonicalization in this PR, or should it be done in a follow-up PR after #67717 is merged?

Zoxc · 2020-01-13T07:58:20Z

Is there something blocking this PR? It holds up #67953.

Aaron1011 · 2020-01-13T10:00:22Z

I assumed that this was waiting on style-servo to be fixed, so that we can do a full perf run.

nikomatsakis · 2020-01-13T13:45:18Z

I have been waiting until we land #67117 -- though if we want to land this first, perhaps some assertions that we don't have inference variables would suffice.

Aaron1011 · 2020-01-14T02:42:37Z

@nikomatsakis: I assume you meant #67717?

nikomatsakis · 2020-01-14T10:52:27Z

Yes. Sorry. :)

Aaron1011 · 2020-01-16T20:28:19Z

Now that the servo benchmarks have been fixed, can I get another perf run?

Mark-Simulacrum · 2020-01-16T21:18:41Z

@bors try @rust-timer queue

rust-timer · 2020-01-16T21:18:43Z

Awaiting bors try build completion

bors · 2020-01-16T21:18:47Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout feature/instance-query (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self feature/instance-query --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging src/librustc/ty/query/keys.rs
Auto-merging src/librustc/ty/mod.rs
CONFLICT (content): Merge conflict in src/librustc/ty/mod.rs
Auto-merging src/librustc/ty/instance.rs
Auto-merging src/librustc/query/mod.rs
Automatic merge failed; fix conflicts and then commit the result.

Aaron1011 · 2020-01-16T22:07:24Z

@Mark-Simulacrum: I've fixed the merge conflict

Mark-Simulacrum · 2020-01-16T22:08:07Z

@bors try

bors · 2020-01-16T22:08:23Z

⌛ Trying commit e73f7a7af4cff1c3af953db35272bcd9812622ae with merge b3fa52e9b78a50cbeec31bbb22ea6ec6209bdd77...

Aaron1011 · 2020-03-19T20:40:05Z

@Dylan-DPC: This should be S-waiting-on-review - I've responded to the most recent round of review comments.

Dylan-DPC-zz · 2020-03-19T20:42:46Z

@Aaron1011 thanks. made the change (Btw you can change labels with rustbot if you don't have access :)

Aaron1011 · 2020-03-30T18:40:16Z

@JohnCSimon: See my previous comment - this should be S-waiting-on-review

Aaron1011 · 2020-04-02T21:18:18Z

@nikomatsakis: Are there any changes you'd like me to make, or should I just rebase this?

nikomatsakis · 2020-04-03T18:36:45Z

@Aaron1011 sorry for delay. r=me when rebased.

Aaron1011 · 2020-04-05T05:24:00Z

@bors r=nikomatsakis

bors · 2020-04-05T05:24:01Z

📌 Commit 63d6ef6 has been approved by nikomatsakis

bors · 2020-04-05T06:12:54Z

⌛ Testing commit 63d6ef6 with merge 84cff50957e27f6a9f18fd6f836cc35ced665e75...

Dylan-DPC-zz · 2020-04-05T06:22:31Z

@bors retry yield

@ghost

Rollup of 5 pull requests Successful merges: - rust-lang#67797 (Query-ify Instance::resolve) - rust-lang#70777 (Don't import integer and float modules, use assoc consts) - rust-lang#70795 (Keep track of position when deleting from a BTreeMap) - rust-lang#70812 (Do not use "nil" to refer to `()`) - rust-lang#70815 (Enable layout debugging for `impl Trait` type aliases) Failed merges: r? @ghost

bors · 2020-04-05T22:48:55Z

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

eddyb · 2020-04-08T08:08:36Z

src/librustc_middle/query/mod.rs

@@ -1257,5 +1257,9 @@ rustc_queries! {
            eval_always
            desc { "looking up enabled feature gates" }
        }
+
+        query resolve_instance(key: (ty::ParamEnv<'tcx>, DefId, SubstsRef<'tcx>)) -> Option<ty::Instance<'tcx>> {


Just spotted this: whenever you have a ty::ParamEnv and some thing referring the to same parameters it adds bounds in scope of, like (DefId, SubstsRef) here, the right thing to do is to use ty::ParamEnvAnd<(DefId, SubstsRef)>.

Doing so should result in better caching (as the bounds in scope aren't kept if they're not needed by the SubtsRef), and is more semantically meaningful.

Will do this in a drive-by commit.

eddyb · 2020-04-08T08:09:50Z

src/librustc_middle/ty/query/keys.rs

@@ -296,3 +296,14 @@ impl Key for (Symbol, u32, u32) {
        DUMMY_SP
    }
 }
+
+impl<'tcx> Key for (ty::ParamEnv<'tcx>, DefId, SubstsRef<'tcx>) {


I guess a strong hint is that you had to add a new Key impl: generally, you shouldn't need one.

rust-highfive assigned estebank Jan 2, 2020

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

rust-highfive assigned nikomatsakis and unassigned estebank Jan 2, 2020

nikomatsakis reviewed Jan 3, 2020

View reviewed changes

cjgillot mentioned this pull request Jan 7, 2020

Split librustc::{traits,infer} to a separate crate rustc_infer #67953

Merged

Aaron1011 mentioned this pull request Jan 14, 2020

Normalize all opaque types when converting ParamEnv to Reveal::All #65989

Merged

Aaron1011 force-pushed the feature/instance-query branch from d1eb2de to e73f7a7 Compare January 16, 2020 22:06

Dylan-DPC-zz added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 19, 2020

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

JohnCSimon 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 28, 2020

This comment has been minimized.

Sign in to view

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

Query-ify Instance::resolve

63d6ef6

Aaron1011 force-pushed the feature/instance-query branch from 2bbc172 to 63d6ef6 Compare April 5, 2020 05:22

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2020

Dylan-DPC-zz mentioned this pull request Apr 5, 2020

Rollup of 5 pull requests #70816

Merged

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 5, 2020

bors merged commit 829154f into rust-lang:master Apr 5, 2020

eddyb reviewed Apr 8, 2020

View reviewed changes

Query-ify Instance::resolve #67797

Query-ify Instance::resolve #67797

Conversation

Aaron1011 commented Jan 2, 2020

rust-highfive commented Jan 2, 2020

Centril commented Jan 2, 2020

rust-timer commented Jan 2, 2020

bors commented Jan 2, 2020

bors commented Jan 2, 2020

rust-timer commented Jan 2, 2020

rust-timer commented Jan 2, 2020

Aaron1011 commented Jan 2, 2020

Mark-Simulacrum commented Jan 2, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Jan 3, 2020

Choose a reason for hiding this comment

Aaron1011 Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Jan 3, 2020

Choose a reason for hiding this comment

Aaron1011 Jan 3, 2020

Choose a reason for hiding this comment

Zoxc commented Jan 13, 2020

Aaron1011 commented Jan 13, 2020

nikomatsakis commented Jan 13, 2020

Aaron1011 commented Jan 14, 2020

nikomatsakis commented Jan 14, 2020

Aaron1011 commented Jan 16, 2020 • edited Loading

Mark-Simulacrum commented Jan 16, 2020

rust-timer commented Jan 16, 2020

bors commented Jan 16, 2020

Aaron1011 commented Jan 16, 2020

Mark-Simulacrum commented Jan 16, 2020

bors commented Jan 16, 2020

Aaron1011 commented Mar 19, 2020

Dylan-DPC-zz commented Mar 19, 2020

Aaron1011 commented Mar 30, 2020

This comment has been minimized.

Aaron1011 commented Apr 2, 2020

nikomatsakis commented Apr 3, 2020

Aaron1011 commented Apr 5, 2020

bors commented Apr 5, 2020

bors commented Apr 5, 2020

Dylan-DPC-zz commented Apr 5, 2020

bors commented Apr 5, 2020

eddyb Apr 8, 2020

Choose a reason for hiding this comment

eddyb Apr 10, 2020

Choose a reason for hiding this comment

eddyb Apr 8, 2020

Choose a reason for hiding this comment

Aaron1011 Jan 3, 2020 •

edited

Loading

Aaron1011 commented Jan 16, 2020 •

edited

Loading