Skip to content

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

New issue

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

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

Already on GitHub? # to your account

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
Collaborator

bors commented Oct 22, 2020

⌛ Trying commit cd45acc with merge d56627bbd1379c41d40dcd6b463e1af38068fb3c...

@bors
Copy link
Collaborator

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
# for free to join this conversation on GitHub. Already have an account? # 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