Use a single function for query manipulations #77833

cjgillot · 2020-10-11T20:56:51Z

There are three ways to invoke a query: get the value, ensure it runs, or have it automatically forced.

The three corresponding methods are merged into a single point of entry.
The goal is to increase the code sharing between the three cases.

rust-highfive · 2020-10-11T20:56:54Z

r? @varkor

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

Xanewok · 2020-10-12T07:55:23Z

I vaguely recall that this may have affected performance (e.g. see careful #[inline(never)] and #[inline(always)] annotations), let's see if this has any impact:

@bors try @rust-timer queue

rust-timer · 2020-10-12T07:55:25Z

Awaiting bors try build completion

bors · 2020-10-12T07:55:40Z

⌛ Trying commit fd60635 with merge f8ec1548e1d547c63153b5e662eda2db7306b2f2...

bors · 2020-10-12T08:38:36Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: f8ec1548e1d547c63153b5e662eda2db7306b2f2 (f8ec1548e1d547c63153b5e662eda2db7306b2f2)

rust-timer · 2020-10-12T08:38:38Z

Queued f8ec1548e1d547c63153b5e662eda2db7306b2f2 with parent 63962f0, future comparison URL.

rust-timer · 2020-10-12T13:40:58Z

Finished benchmarking try commit (f8ec1548e1d547c63153b5e662eda2db7306b2f2): 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 rollup- to bors.

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

Julian-Wollersberger

This refactoring should improve bootstrap times a bit, because it removes 216 000 llvm-lines (-1%).

Before:  Lines   Copies         Function name
--------------   ------         -------------
20218434 (100%)  600060 (100%)  (TOTAL)
  457651 (2.3%)     414 (0.1%)  rustc_query_system::query::plumbing::get_query_impl::{{closure}}
  168207 (0.8%)     354 (0.1%)  rustc_query_system::query::plumbing::force_query_impl::{{closure}}
   95634 (0.5%)     207 (0.0%)  rustc_query_system::query::plumbing::incremental_verify_ich
   87688 (0.4%)    4335 (0.7%)  core::ops::function::FnOnce::call_once
   75830 (0.4%)     621 (0.1%)  <rustc_query_system::query::caches::DefaultCache<K,V> as rustc_query_system::query::caches::QueryCache>::iter
   69233 (0.3%)     207 (0.0%)  rustc_query_system::query::plumbing::load_from_disk_and_cache_in_memory
   41047 (0.2%)     207 (0.0%)  rustc_query_system::query::plumbing::get_query_impl

After:  Lines    Copies         Function name
-------------    ------         -------------
20001669 (100%)  595297 (100%)  (TOTAL)
  434107 (2.2%)     354 (0.1%)  rustc_query_system::query::plumbing::call_query_impl::{{closure}}
   85516 (0.4%)    4257 (0.7%)  core::ops::function::FnOnce::call_once
   81774 (0.4%)     177 (0.0%)  rustc_query_system::query::plumbing::incremental_verify_ich
   75830 (0.4%)     621 (0.1%)  <rustc_query_system::query::caches::DefaultCache<K,V> as rustc_query_system::query::caches::QueryCache>::iter
   59176 (0.3%)     177 (0.0%)  rustc_query_system::query::plumbing::load_from_disk_and_cache_in_memory
   47042 (0.2%)     177 (0.0%)  rustc_query_system::query::plumbing::call_query_impl

(Omitted all entries from other crates; mostly core, alloc and hashbrown.)

Julian-Wollersberger · 2020-10-12T12:05:41Z

compiler/rustc_middle/src/ty/query/plumbing.rs

+                key: query_keys::$name<$tcx>,
+                caller: QueryCaller<DepKind>,
+            ) -> Option<<queries::$name<$tcx> as QueryConfig<TyCtxt<$tcx>>>::Stored> {
+                call_query::<queries::$name<'_>, _>(tcx, span, key, caller)


Why is this wrapper needed? Can't the three places where this is used call call_query::<queries::$name<'_>, _>(tcx, span, key, caller)directly?

The point is to monomorphize call_query only once for each query. The easiest way is to call it through a monomorphic function.

I just tested removing the wrapper. It removed another 39 022 llvm-lines.
I guess call_query is only monomorphized once for each query anyway, even though it is called from multiple places.

Edit: Created cjgillot#1

Julian-Wollersberger · 2020-10-12T12:13:49Z

compiler/rustc_query_system/src/query/plumbing.rs

-) where
-    C: QueryCache,
-    C::Key: Eq + Clone + crate::dep_graph::DepNodeParams<CTX>,
+fn ensure_query_impl<CTX, K, V>(tcx: CTX, key: &K, query: &QueryVtable<CTX, K, V>) -> bool


Can you add a comment what this bool means?

Julian-Wollersberger · 2020-10-12T13:14:56Z

compiler/rustc_query_system/src/query/plumbing.rs

 where
-    C: QueryCache,


Glad to see this C: QueryCache clause removed ^^
It is annoying when trying to make things dyn.

cjgillot · 2020-10-12T19:57:29Z

LLVM-lines was the target, but perf is bad. Bootstrap times are not a win either. This definitely needs more work.

bors · 2020-10-22T15:05:12Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-10-25T00:47:34Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

cjgillot · 2020-11-20T22:16:19Z

Closed in favor of #78780.

rust-highfive assigned varkor Oct 11, 2020

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

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 11, 2020

Julian-Wollersberger suggested changes Oct 12, 2020

View reviewed changes

jyn514 added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Oct 12, 2020

Julian-Wollersberger mentioned this pull request Oct 13, 2020

Removed the wrapper around call_query(). cjgillot/rust#1

Merged

cjgillot force-pushed the query-merge branch from 62efb4d to f2ea6ba Compare October 21, 2020 20:43

cjgillot and others added 10 commits October 22, 2020 19:25

Merge {get,ensure,force}_query into call_query.

addc880

Move get_query_impl out of ensure_query_impl.

3e23b76

Remove {get,force}_query_impl.

3f66069

Refactor logic into try_execute_query.

c986f6d

Move job.complete out of force_query_with_job.

7fddbde

Move job creation/completion out of try_execute_query.

d4bf8fb

Removed the wrapper around call_query().

e256d22

Add comment.

945d588

Remove useless bounds.

4d9e839

Simplify forcing.

99d65b0

cjgillot force-pushed the query-merge branch from f2ea6ba to 99d65b0 Compare October 22, 2020 21:24

varkor removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2020

varkor added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 23, 2020

cjgillot closed this Nov 20, 2020

cjgillot deleted the query-merge branch September 21, 2021 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a single function for query manipulations #77833

Use a single function for query manipulations #77833

cjgillot commented Oct 11, 2020 •

edited by jyn514

Loading

rust-highfive commented Oct 11, 2020

Xanewok commented Oct 12, 2020

rust-timer commented Oct 12, 2020

bors commented Oct 12, 2020

bors commented Oct 12, 2020

rust-timer commented Oct 12, 2020

rust-timer commented Oct 12, 2020

Julian-Wollersberger left a comment •

edited

Loading

Julian-Wollersberger Oct 12, 2020

cjgillot Oct 12, 2020

Julian-Wollersberger Oct 13, 2020 •

edited

Loading

Julian-Wollersberger Oct 12, 2020

Julian-Wollersberger Oct 12, 2020

cjgillot commented Oct 12, 2020

bors commented Oct 22, 2020

bors commented Oct 25, 2020

cjgillot commented Nov 20, 2020

Use a single function for query manipulations #77833

Use a single function for query manipulations #77833

Conversation

cjgillot commented Oct 11, 2020 • edited by jyn514 Loading

rust-highfive commented Oct 11, 2020

Xanewok commented Oct 12, 2020

rust-timer commented Oct 12, 2020

bors commented Oct 12, 2020

bors commented Oct 12, 2020

rust-timer commented Oct 12, 2020

rust-timer commented Oct 12, 2020

Julian-Wollersberger left a comment • edited Loading

Choose a reason for hiding this comment

Julian-Wollersberger Oct 12, 2020

Choose a reason for hiding this comment

cjgillot Oct 12, 2020

Choose a reason for hiding this comment

Julian-Wollersberger Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Julian-Wollersberger Oct 12, 2020

Choose a reason for hiding this comment

Julian-Wollersberger Oct 12, 2020

Choose a reason for hiding this comment

cjgillot commented Oct 12, 2020

bors commented Oct 22, 2020

bors commented Oct 25, 2020

cjgillot commented Nov 20, 2020

cjgillot commented Oct 11, 2020 •

edited by jyn514

Loading

Julian-Wollersberger left a comment •

edited

Loading

Julian-Wollersberger Oct 13, 2020 •

edited

Loading