Take `impl Into<DefId>` for query methods on `TyCtxt` #70961

ecstatic-morse · 2020-04-09T17:33:43Z

Alternative implementation of #70956. cc #70853.

rust-highfive · 2020-04-09T17:33:47Z

r? @petrochenkov

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

ecstatic-morse · 2020-04-09T17:34:07Z

r? @Zoxc

ecstatic-morse · 2020-04-09T17:34:42Z

cc @marmeladema @eddyb

eddyb · 2020-04-09T17:43:45Z

src/librustc_middle/ty/query/plumbing.rs

+            fn inner(this: TyCtxtAt<$tcx>, key: DefId) -> $V {
+                get_query::<queries::$name<'_>, _>(this.tcx, this.span, key)
+            }


Is get_query inlined? This does nothing otherwise.

This was an attempt to address @Zoxc's comment. I don't understand why it's beneficial for the caller of get_query to be monomorphic, since it is #[inline(never)]. Perhaps I misunderstood? ensure_query is not #[inline(never)] at the moment, however.

IFAIK, the LLVM codegen of the {get,force,ensure}_query functions is responsible for most of the compile time for librustc_middle. It happens once for each of the 170 queries. By making the caller generic, part of this codegen may be replicated in caller crates. This would have a serious impact on rustc's compile time. Please correct me if #[inline(never)] is enough to prevent this.

The wrapper functions are already #[inline(always)] so they would be codegen'd cross-crate even when not generic.

eddyb · 2020-04-09T17:44:25Z

src/librustc_middle/ty/query/plumbing.rs

+    (TyCtxtAt<$tcx:tt>, $(#[$attr:meta])* $name:ident(DefId) -> $V:ty) => {
+        $(#[$attr])*
+        #[inline(always)]
+        pub fn $name(self, key: impl Into<DefId>) -> $V {


Can you make a type macro and use that to pattern-match on just the key type?
I think we can just use .into() in all methods, as it will compile to a noop when the types are the same.

That's a good idea. It won't work if we do want a monomorphic helper function, but maybe we don't.

get_query is #[inline(never)] (I just checked) so you don't need this wrapper.

nikomatsakis · 2020-04-10T17:27:38Z

Per rust-lang/team#316, r? @eddyb -- let me know @eddyb if you'd prefer someone else.

ecstatic-morse · 2020-04-10T20:56:46Z

@eddyb The last two commits implement your suggestion for simplifying the macro and replace Into with a newly defined IntoQueryParam respectively. You can't seal traits with blanket impls, although I suppose since this is only used for DefId we could just have manual impls.

eddyb · 2020-04-10T21:00:56Z

src/librustc_middle/ty/query/mod.rs

+pub trait IntoQueryParam<P> {
+    fn into_query_param(self) -> P;
+}


In case there was any confusion, "sealing" works like this:

mod sealed { pub trait IntoQueryParam<P> { fn into_query_param(self) -> P; } }

Then you can refer to it as sealed::IntoQueryParam, and as long as sealed remains private, and you don't reexport IntoQueryParam somewhere, it should be impossible to implement or import the trait, outside of the parent module of the mod sealed.

Ah, I thought you meant the super-trait patten that's in the API guidelines.

@ecstatic-morse ah I see, that pattern uses a sealed trait for a specific purpose.
Or maybe I'm the only person referring to "pub but in a private mod" as "sealed".

src/librustc_span/def_id.rs

eddyb · 2020-04-10T21:20:08Z

LGTM, but before I approve:

@bors try @rust-timer queue

rust-timer · 2020-04-10T21:20:09Z

Awaiting bors try build completion

bors · 2020-04-10T21:20:18Z

⌛ Trying commit 3ac350d8349c9fbb921c9b9ac7f01cf5c8869759 with merge bca1f3800d78009dd283fff15b216fa1edf32996...

eddyb · 2020-04-10T21:24:43Z

src/librustc_middle/ty/query/plumbing.rs

-            pub fn $name(self, key: $K) {
-                ensure_query::<queries::$name<'_>, _>(self.tcx, key)
+            pub fn $name(self, key: query_helper_param_ty!($($K)*)) {
+                ensure_query::<queries::$name<'_>, _>(self.tcx, key.into_query_param())


Actually, are these changes needed? "ensuring" should be rather rare AFAIK?

…e-local-def-id, r=eddyb rustc_middle: return `LocalDefId` where possible in hir::map module This changes the return type of the following functions to return a `LocalDefId` instead of a `DefId`: * opt_local_def_id_from_node_id * opt_local_def_id * body_owner_def_id * local_def_id_from_node_id * get_parent_id This is another step in the right direction for rust-lang#70853 This pull request will be followed by another (substantial one) which changes the return type of `local_def_id` function but this change being more invasive, we might want to wait for rust-lang#70956 or rust-lang#70961 (or some other form it) to land first.

…dle-local-def-id, r=eddyb rustc_middle: return `LocalDefId` where possible in hir::map module This changes the return type of the following functions to return a `LocalDefId` instead of a `DefId`: * opt_local_def_id_from_node_id * opt_local_def_id * body_owner_def_id * local_def_id_from_node_id * get_parent_id This is another step in the right direction for rust-lang#70853 This pull request will be followed by another (substantial one) which changes the return type of `local_def_id` function but this change being more invasive, we might want to wait for rust-lang#70956 or rust-lang#70961 (or some other form it) to land first.

bors · 2020-04-10T23:50:21Z

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

rust-timer · 2020-04-10T23:50:23Z

Queued bca1f3800d78009dd283fff15b216fa1edf32996 with parent 9682f0e, future comparison URL.

rust-timer · 2020-04-11T03:07:37Z

Finished benchmarking try commit bca1f3800d78009dd283fff15b216fa1edf32996, comparison URL.

marmeladema · 2020-04-11T13:46:18Z

@ecstatic-morse @eddyb is the performance good enough for this to land?

ecstatic-morse · 2020-04-11T16:29:54Z

@marmeladema Too slow IMO. We'll need to micro-optimize or abandon this idea.

eddyb · 2020-04-11T23:01:02Z

src/librustc_middle/ty/query/mod.rs

+    }
+
+    impl IntoQueryParam<DefId> for LocalDefId {
+        fn into_query_param(self) -> DefId {


Try adding #[inline(always)] to this (and to_def_id if it doesn't have it).

Although, wait, are we using this at all now? Is the slowdown just from going through the noop impl?

It's used exactly once as a test 6e5a210. I'm skeptical that this will help, since no into_query_param call appears in the optimized binary, and the DefId queries I looked at inline down to get_query. There's no predicting the optimizer though.

ecstatic-morse · 2020-04-11T23:40:28Z

@bors try
@rust-timer queue

rust-timer · 2020-04-11T23:40:29Z

Awaiting bors try build completion

bors · 2020-04-11T23:40:41Z

⌛ Trying commit 04c91a0 with merge 4db70335bb09ba1a20ddee9e96cc14d6bb67b630...

bors · 2020-04-12T02:15:34Z

☀️ Try build successful - checks-azure
Build commit: 4db70335bb09ba1a20ddee9e96cc14d6bb67b630 (4db70335bb09ba1a20ddee9e96cc14d6bb67b630)

rust-timer · 2020-04-12T02:15:36Z

Queued 4db70335bb09ba1a20ddee9e96cc14d6bb67b630 with parent e82734e, future comparison URL.

rust-timer · 2020-04-12T13:13:14Z

Finished benchmarking try commit 4db70335bb09ba1a20ddee9e96cc14d6bb67b630, comparison URL.

eddyb · 2020-04-12T13:15:04Z

Heh that's what it was! r=me if there is no disagreement about doing this.

eddyb · 2020-04-13T12:25:56Z

Eh let's just do it.

@bors r+

bors · 2020-04-13T12:25:57Z

📌 Commit 04c91a0 has been approved by eddyb

bors · 2020-04-13T12:50:49Z

⌛ Testing commit 04c91a0 with merge 5179ebe...

bors · 2020-04-13T16:05:32Z

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 5179ebe to master...

rust-highfive assigned petrochenkov Apr 9, 2020

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

rust-highfive assigned Zoxc and unassigned petrochenkov Apr 9, 2020

ecstatic-morse mentioned this pull request Apr 9, 2020

librustc_middle: change query functions to accept impl Into<$K> type #70956

Closed

eddyb reviewed Apr 9, 2020

View reviewed changes

ecstatic-morse force-pushed the into-def-id branch from 6ed806b to e1de311 Compare April 9, 2020 17:50

marmeladema mentioned this pull request Apr 10, 2020

rustc_middle: return LocalDefId where possible in hir::map module #70986

Merged

nikomatsakis unassigned Zoxc Apr 10, 2020

rust-highfive assigned eddyb Apr 10, 2020

ecstatic-morse force-pushed the into-def-id branch from e1de311 to 5ff63c9 Compare April 10, 2020 20:54

eddyb reviewed Apr 10, 2020

View reviewed changes

src/librustc_span/def_id.rs Outdated Show resolved Hide resolved

eddyb approved these changes Apr 10, 2020

View reviewed changes

eddyb reviewed Apr 10, 2020

View reviewed changes

ecstatic-morse added 7 commits April 11, 2020 15:18

Make query helpers on TyCtxt take impl Into<DefId>

62b5aff

Impl From<LocalDefId> for DefId

10ed501

Fix inference fallout

0d4639d

Remove call to to_def_id before calling query helper

6e5a210

Simplify macro

fd8cf3c

Use custom trait instead of Into

beeacdb

Apply suggestions from review

f0c7733

eddyb reviewed Apr 11, 2020

View reviewed changes

Add #[inline(always)] to into_query_param

04c91a0

ecstatic-morse force-pushed the into-def-id branch from 3ac350d to 04c91a0 Compare April 11, 2020 23:31

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 13, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 13, 2020

bors merged commit 5179ebe into rust-lang:master Apr 13, 2020

bors mentioned this pull request Apr 13, 2020

Move the query engine out of rustc_middle #70951

Merged

ecstatic-morse deleted the into-def-id branch October 6, 2020 01:42

Take impl Into<DefId> for query methods on TyCtxt #70961

Take impl Into<DefId> for query methods on TyCtxt #70961

Conversation

ecstatic-morse commented Apr 9, 2020

rust-highfive commented Apr 9, 2020

ecstatic-morse commented Apr 9, 2020

ecstatic-morse commented Apr 9, 2020

Choose a reason for hiding this comment

ecstatic-morse Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Apr 10, 2020

ecstatic-morse commented Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Apr 10, 2020

rust-timer commented Apr 10, 2020

bors commented Apr 10, 2020

Choose a reason for hiding this comment

bors commented Apr 10, 2020

rust-timer commented Apr 10, 2020

rust-timer commented Apr 11, 2020

marmeladema commented Apr 11, 2020

ecstatic-morse commented Apr 11, 2020

Choose a reason for hiding this comment

ecstatic-morse Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse commented Apr 11, 2020

rust-timer commented Apr 11, 2020

bors commented Apr 11, 2020

bors commented Apr 12, 2020

rust-timer commented Apr 12, 2020

rust-timer commented Apr 12, 2020

eddyb commented Apr 12, 2020

eddyb commented Apr 13, 2020

bors commented Apr 13, 2020

bors commented Apr 13, 2020

bors commented Apr 13, 2020

Take `impl Into<DefId>` for query methods on `TyCtxt` #70961

Take `impl Into<DefId>` for query methods on `TyCtxt` #70961

ecstatic-morse Apr 9, 2020 •

edited

Loading

ecstatic-morse commented Apr 10, 2020 •

edited

Loading

ecstatic-morse Apr 11, 2020 •

edited

Loading