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

Revert "Monomorphize calculate_dtor instead of using function pointers" #78221

Closed

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 22, 2020

Reverts #77755

There is a chance that this PR caused an unexpected slowdown. For some reason, the try and merge perf measurements differ significantly.

r? @Mark-Simulacrum

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

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 22, 2020

⌛ Trying commit cd45acc with merge d56627bbd1379c41d40dcd6b463e1af38068fb3c...

@bors
Copy link
Contributor

bors commented Oct 22, 2020

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

@rust-timer
Copy link
Collaborator

Queued d56627bbd1379c41d40dcd6b463e1af38068fb3c with parent 6b9fbf2, future comparison URL.

@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 Oct 22, 2020
@rust-timer
Copy link
Collaborator

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

@bugadani
Copy link
Contributor Author

bugadani commented Oct 22, 2020

Well at least this isn't the 1.x% regression it was measured to be. But, what was that, then? Also, does this mean a random PR got a positive perf result as well?

@Mark-Simulacrum
Copy link
Member

Probably partially noise, partially other work may have fixed some of the regressions introduced then. I'm going to close this as I don't think it's worth reverting the original patch on performance grounds given these results.

Also note that the percentage here is "of" a different value, so comparing it directly is likely flawed anyway.

@bugadani bugadani deleted the revert-77755-perf-calc-dtor branch October 22, 2020 17:54
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.

6 participants