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

[DO NOT MERGE] Experiment: disable the niche optimization for most enums #77816

Closed
wants to merge 1 commit into from

Conversation

ogoffart
Copy link
Contributor

(Only keep the niche optimization for small enum to avoid cases where the size would be doubling)

PR #70477 and #75866 have shown that trying to optimization more often lead to performance regressions.
It would be interesting to know if there would be performance improvements by using the niche optimization less often.

My hypotheses is that the generated code for match when using the niche optimization is sub-optimal and should deserve to be optimized. This experiment should show how much speed we could expect to gain by optimizing it.

I'm kindly asking for a pref run of this PR.

cc @eddyb @erikdesjardins

(Only keep it for every small enum to avoid cases where the size is doubling)

Trying to use the niche optimisation more often lead to performence regression
It would be interresting to know what would be the performence improvement by not
having the niche optimization.
@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

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

jyn514 commented Oct 11, 2020

@bors try @rust-timer queue

I'm curious whether this works ...

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 11, 2020

⌛ Trying commit a8ea6b7 with merge 7273f5a95a6ff81024f115bd286ea713bce5123b...

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 11, 2020

⌛ Trying commit a8ea6b7 with merge 4db03f51dda11b446970cad86f1e71b517010e74...

@jyn514
Copy link
Member

jyn514 commented Oct 11, 2020

🤦 @rust-lang/infra apparently bors and rust-timer will queue twice if I edit the comment, is there a way to fix that?

@rust-log-analyzer
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Oct 11, 2020

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

@rust-timer
Copy link
Collaborator

Queued 4db03f51dda11b446970cad86f1e71b517010e74 with parent c38f001, future comparison URL.

@jyn514 jyn514 added A-layout Area: Memory layout of types 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 11, 2020
@rust-timer
Copy link
Collaborator

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

@ogoffart
Copy link
Contributor Author

The result show a small performance improvement, and also some small regressions. I guess there is two factor at play: the gain because the match branch is easier, and the loss because some enum gets bigger. I'm not quite sure how to interpret the result. But I believe the gain would be what we should expect by optimizing the match code generation for niche enum. I guess the difference is not as big as the regression in #70477 because the niche optimization can be used in relatively few enum currently, while #70477 would enable it with many more enum (i think)

I'm closing this PR and the discussion can continue on #75866

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Area: Memory layout of types 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.

7 participants