Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make fewer types generic over QueryContext #77871

Merged
merged 2 commits into from
Oct 22, 2020

Conversation

Julian-Wollersberger
Copy link
Contributor

@Julian-Wollersberger Julian-Wollersberger commented Oct 12, 2020

While trying to refactor rustc_query_system::query::QueryContext to make it dyn-safe, I noticed some smaller things:

  • QueryConfig doesn't need to be generic over QueryContext
  • The kind field on QueryJobId is unused
  • Some unnecessary where clauses
  • Many types in job.rs where generic over QueryContext but only needed QueryContext::Query.
    If handle_cycle_error() could be refactored to not take error: CycleError<CTX::Query>, all those bounds could be removed as well.

Changing find_cycle_in_stack() in job.rs to not take a tcx argument is the only functional change here. Everything else is just updating type signatures. (aka compile-error driven development ^^)

Currently there is a weird bug where memory usage suddenly skyrockets when running UI tests. I'll investigate that tomorrow.
A perf run probably won't make sense before that is fixed.

EDIT: kind actually is used by Eq, and re-adding it fixed the memory issue.

@rust-highfive
Copy link
Collaborator

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2020
@varkor
Copy link
Member

varkor commented Oct 12, 2020

I'm not very familiar with the query system, so someone like r? @oli-obk would be a better reviewer.

@rust-highfive rust-highfive assigned oli-obk and unassigned varkor Oct 12, 2020
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 12, 2020

⌛ Trying commit fe06f1a6a1d439eb5724ce9d83f6ad395dc83475 with merge 9a6fc604ceb395c2682b429150a3642bc9b30ce1...

@jyn514 jyn514 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiletime Issue: Problems and improvements with respect to compile times. A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) labels Oct 12, 2020
@bors
Copy link
Contributor

bors commented Oct 12, 2020

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

@rust-timer
Copy link
Collaborator

Queued 9a6fc604ceb395c2682b429150a3642bc9b30ce1 with parent f3ab6f0, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (9a6fc604ceb395c2682b429150a3642bc9b30ce1): 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

@mati865
Copy link
Contributor

mati865 commented Oct 13, 2020

Slight regression across the board.

@Julian-Wollersberger
Copy link
Contributor Author

This PR has given me headaches, but now it should be ready.

First, I undid the removal of the kind field of QueryJobId. This one was the cause of the memory bug,
but I still don't understand how that happened. The field wasn't read anywhere.
EDIT: As @cjgillot mentioned, it's probably the #[derive(Eq)].

Measuring where the perf regression came from was also tricky, because my two identical checkouts of rustc turned out to not be identical. Sight. Passing Option<QueryJobId> by value turned out to be the culprit.

@cjgillot
Copy link
Contributor

This crate needs some convention for the type parameters. The amount of those is getting out of hand.

@Julian-Wollersberger
Copy link
Contributor Author

I feel you, since I've been doing type juggling for hours on end.
Let's see if I can make kind: Box<dyn DepKind> in QueryJobId.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 15, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 15, 2020

⌛ Trying commit a4345bd9ba293bb256eb7882993dab550248368d with merge fdeb035d22cabe983e97a8b865f30c5701ff3a22...

@bors
Copy link
Contributor

bors commented Oct 15, 2020

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

@rust-timer
Copy link
Collaborator

Queued fdeb035d22cabe983e97a8b865f30c5701ff3a22 with parent 7f58716, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (fdeb035d22cabe983e97a8b865f30c5701ff3a22): 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
Copy link
Contributor Author

Julian-Wollersberger commented Oct 16, 2020

Performance is neutral.

I looked into dyn DepKind, which was also suggested by @oli-obk on Zulip, but it's not easy since DepKind implements Clone + Eq + Ord + Hash which are all not dyn safe.
So it's probably better to do that in a separate PR.

With that, can you review it @oli-obk?
It's probably best to review the total changes, since three commits mostly touch the same lines. (Rebasing would be painful...)
It's mostly type signature changes, and I changed the arguments to find_cycle_in_stack() and pick_query() in job.rs.

tcx: CTX,
key: K,
prev_dep_node_index: SerializedDepNodeIndex,
dep_node_index: DepNodeIndex,
dep_node: &DepNode<CTX::DepKind>,
query: &QueryVtable<CTX, K, V>,
) -> V
where
CTX: QueryContext,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these changes moving bounds from where bounds to generic arg declarations are not necessary for this PR. Please revert them. If I remember correctly this is all on purpose, let's not change it in a drive-by manner.

Copy link
Contributor Author

@Julian-Wollersberger Julian-Wollersberger Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I also change it when there was no where clause but now the generic args are a lot longer?
Like

impl<D: Copy + Clone + Eq + Hash, Q: Clone> QueryJob<D, Q> {

to

impl<D, Q> QueryJob<D, Q>
where
    D: Copy + Clone + Eq + Hash,
    Q: Clone,

EDIT: I've done that.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 16, 2020

Except for the stylistic nit, this lgtm

@cjgillot
Copy link
Contributor

Could you remove the two QueryJobId commits? The back-and-forth changes the type parameters needlessly.

@Julian-Wollersberger
Copy link
Contributor Author

Could you remove the two QueryJobId commits? The back-and-forth changes the type parameters needlessly.

The type is changed from CXT: QueryContext to D.
If it isn't to annoying to you, I wouldn't do it because squashing the commits would be a pain. Although CLion has this neat "Interactively rebase from here" feature, I'd still have to resolve merge conflicts in 7 files multiplied by 3 commits.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 17, 2020

because squashing the commits would be a pain

Why is squashing a problem? Or do you mean, move the commits around, then squash? Just straight sqashing sequential commits never causes any merge conflicts

It was only needed by `find_cycle_in_stack()` in job.rs, but needed to be forwarded through dozens of types.
@Julian-Wollersberger
Copy link
Contributor Author

Julian-Wollersberger commented Oct 19, 2020

Yes, I meant moving commits around. I was concerned because otherwise it would squash to many commits.
So I guess it's better to squash to much than to little? Whats the policy here?

Anyway, I squashed everything but the first commit. Sorry for the back and forth.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 19, 2020

@bors r+

thanks!

@bors
Copy link
Contributor

bors commented Oct 19, 2020

📌 Commit 52cedca has been approved by oli-obk

@bors 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 Oct 19, 2020
@Julian-Wollersberger
Copy link
Contributor Author

This PR could be rolled up, right? Because it's perf neutral

@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

@bors rollup=iffy

sure, but it also touches a lot of stuff, so it conflicts a lot

@bors
Copy link
Contributor

bors commented Oct 22, 2020

⌛ Testing commit 52cedca with merge 500ddc5...

@bors
Copy link
Contributor

bors commented Oct 22, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 500ddc5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 22, 2020
@bors bors merged commit 500ddc5 into rust-lang:master Oct 22, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) I-compiletime Issue: Problems and improvements with respect to compile times. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.