- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
Make fewer types generic over QueryContext #77871
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
Conversation
| r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) | 
| I'm not very familiar with the query system, so someone like r? @oli-obk would be a better reviewer. | 
| @bors try @rust-timer queue | 
| Awaiting bors try build completion | 
| ⌛ Trying commit fe06f1a6a1d439eb5724ce9d83f6ad395dc83475 with merge 9a6fc604ceb395c2682b429150a3642bc9b30ce1... | 
| ☀️ Try build successful - checks-actions, checks-azure | 
| Queued 9a6fc604ceb395c2682b429150a3642bc9b30ce1 with parent f3ab6f0, future comparison URL. | 
| 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  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 | 
| Slight regression across the board. | 
c38506b    to
    fa58a94      
    Compare
  
    | This PR has given me headaches, but now it should be ready. First, I undid the removal of the  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  | 
e4db3f6    to
    a4345bd      
    Compare
  
    | This crate needs some convention for the type parameters. The amount of those is getting out of hand. | 
| I feel you, since I've been doing type juggling for hours on end. | 
| @bors try @rust-timer queue | 
| Awaiting bors try build completion | 
| ⌛ Trying commit a4345bd9ba293bb256eb7882993dab550248368d with merge fdeb035d22cabe983e97a8b865f30c5701ff3a22... | 
| ☀️ Try build successful - checks-actions, checks-azure | 
| Queued fdeb035d22cabe983e97a8b865f30c5701ff3a22 with parent 7f58716, future comparison URL. | 
| 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  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 | 
| Performance is neutral. I looked into  With that, can you review it @oli-obk? | 
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Except for the stylistic nit, this lgtm | 
| Could you remove the two  | 
8548c57    to
    1d8af05      
    Compare
  
    | 
 The type is changed from  | 
| 
 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.
1d8af05    to
    52cedca      
    Compare
  
    | Yes, I meant moving commits around. I was concerned because otherwise it would squash to many commits. Anyway, I squashed everything but the first commit. Sorry for the back and forth. | 
| @bors r+ thanks! | 
| 📌 Commit 52cedca has been approved by  | 
| This PR could be rolled up, right? Because it's perf neutral | 
| @bors rollup=iffy sure, but it also touches a lot of stuff, so it conflicts a lot | 
| ☀️ Test successful - checks-actions, checks-azure | 
While trying to refactor
rustc_query_system::query::QueryContextto make it dyn-safe, I noticed some smaller things:Thekindfield on QueryJobId is unusedjob.rswhere generic overQueryContextbut only neededQueryContext::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 atcxargument 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:
kindactually is used byEq, and re-adding it fixed the memory issue.