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

Inline DebruijnIndex methods #80057

Closed

Conversation

LeSeulArtichaut
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut commented Dec 15, 2020

Attempts to fix the perf regression from #79169. Could someone queue a perf run for this?

r? @nikomatsakis cc @rylev

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

rylev commented Dec 15, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@bors
Copy link
Contributor

bors commented Dec 15, 2020

⌛ Trying commit 2208f5e with merge d556efc2d01383e316fdb5a9cad3ba9df3f9b0fb...

@lqd
Copy link
Member

lqd commented Dec 15, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Dec 15, 2020

⌛ Trying commit 2208f5e with merge c49e4b740ec507a6d372a03481e2990c33c1dade...

@bors
Copy link
Contributor

bors commented Dec 15, 2020

☀️ Try build successful - checks-actions
Build commit: c49e4b740ec507a6d372a03481e2990c33c1dade (c49e4b740ec507a6d372a03481e2990c33c1dade)

@rust-timer
Copy link
Collaborator

Queued c49e4b740ec507a6d372a03481e2990c33c1dade with parent 99baddb, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 15, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c49e4b740ec507a6d372a03481e2990c33c1dade): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 15, 2020
@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Dec 15, 2020

This doesn't seem to fix the performance 😕

@jackh726
Copy link
Member

I wonder if it's the functions on TypeFlags?

@jyn514 jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 16, 2020
@LeSeulArtichaut
Copy link
Contributor Author

I wonder if it's the functions on TypeFlags?

IIUC those already have #[inline] (see e.g. empty)...

@LeSeulArtichaut
Copy link
Contributor Author

What should I do with this? Should I close it? Should we merge it?

@rylev
Copy link
Member

rylev commented Dec 17, 2020

If this isn't improving performance, I don't see why we should merge it. Thoughts?

@LeSeulArtichaut
Copy link
Contributor Author

Yeah, I'll just close this.

@LeSeulArtichaut LeSeulArtichaut deleted the debruijn-inline branch December 17, 2020 15:48
@jackh726
Copy link
Member

With regards to #79169, the only other function that got moved out was the hash_stable on DebruijnIndex. Other than that, there's not a clear target for inlining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

10 participants