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

[PERF][WIP] Use unchecked_add in Layout::repeat #69679

Closed

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Mar 3, 2020

#69635 (a rollup) caused a small performance regression. Nothing in that rollup jumps out at me, but #69544 alters some code that is pretty hot, so let's mess with that.

r? @ghost (just for a perf run)

@ecstatic-morse
Copy link
Contributor Author

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Mar 3, 2020

⌛ Trying commit 94045da with merge 9cea05f9131a9149b2683c42b30e78ccf4c4f45d...

@jonas-schievink
Copy link
Contributor

#69544 changes this from checked_add to +, which I'd expect to improve performance. Very odd. I also can't see anything else in that rollup though.

@bors
Copy link
Contributor

bors commented Mar 4, 2020

☀️ Try build successful - checks-azure
Build commit: 9cea05f9131a9149b2683c42b30e78ccf4c4f45d (9cea05f9131a9149b2683c42b30e78ccf4c4f45d)

@rust-timer
Copy link
Collaborator

Queued 9cea05f9131a9149b2683c42b30e78ccf4c4f45d with parent 3c5b1b7, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9cea05f9131a9149b2683c42b30e78ccf4c4f45d, comparison URL.

@ecstatic-morse
Copy link
Contributor Author

No change. We could try reverting #69544 entirely or adding #[inline(never)] to repeat, but the perf queue is pretty full so I don't want to do too many speculative perf runs. I think I'll close this and open an issue.

@ecstatic-morse ecstatic-morse deleted the optim-layout-repeat branch March 4, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants