Use a single function for query manipulations by cjgillot · Pull Request #77833 · rust-lang/rust

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

+ 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

-) 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

+ query: &QueryVtable<CTX, K, V>,
+) -> (V, DepNodeIndex)
 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 f2ea6baCompare October 21, 2020 20:43

cjgillotand 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 99d65b0Compare 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

Uh oh!

Uh oh!

Conversation

cjgillot commented Oct 11, 2020• edited by jyn514 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Oct 11, 2020

Uh oh!

Xanewok commented Oct 12, 2020

Uh oh!

rust-timer commented Oct 12, 2020

Uh oh!

bors commented Oct 12, 2020

Uh oh!

bors commented Oct 12, 2020

Uh oh!

rust-timer commented Oct 12, 2020

Uh oh!

rust-timer commented Oct 12, 2020

Uh oh!

Julian-Wollersberger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julian-WollersbergerOct 12, 2020

Choose a reason for hiding this comment

Uh oh!

cjgillotOct 12, 2020

Choose a reason for hiding this comment

Uh oh!

Julian-WollersbergerOct 13, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julian-WollersbergerOct 12, 2020

Choose a reason for hiding this comment

Uh oh!

Julian-WollersbergerOct 12, 2020

Choose a reason for hiding this comment

Uh oh!

cjgillot commented Oct 12, 2020

Uh oh!

bors commented Oct 22, 2020

Uh oh!

bors commented Oct 25, 2020

Uh oh!

cjgillot commented Nov 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

cjgillot commented Oct 11, 2020•
edited by jyn514
Loading

Julian-Wollersberger left a comment •
edited
Loading

Julian-WollersbergerOct 13, 2020•
edited
Loading