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

[experiment] ty/layout: compute both niche-filling and tagged layouts for enums. #71045

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 11, 2020

This is something I've wanted to do for a while, since if we can get away with it perf-wise, it should allow us to always pick the better of the two, even when the conditions would be otherwise subtle.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Apr 11, 2020
@eddyb
Copy link
Member Author

eddyb commented Apr 11, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 11, 2020

⌛ Trying commit 13c200a with merge 9a594eaf4326233bc29fe01ece3e52c2a0406229...

@bors
Copy link
Contributor

bors commented Apr 12, 2020

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

@rust-timer
Copy link
Collaborator

Queued 9a594eaf4326233bc29fe01ece3e52c2a0406229 with parent e82734e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9a594eaf4326233bc29fe01ece3e52c2a0406229, comparison URL.

@bjorn3
Copy link
Member

bjorn3 commented Apr 12, 2020

There doesn't seem to be any perf difference between master and this PR.

@petrochenkov
Copy link
Contributor

r? @RalfJung

@RalfJung
Copy link
Member

I'm afraid I don't know this code nearly well enough to do reviews. I'd have suggested @eddyb until I saw they are the PR author. ;)

@eddyb
Copy link
Member Author

eddyb commented Apr 12, 2020

This is only for the perf test 📈. If we did land this, we'd add a comparison between the two layouts, and pick the one that has smaller size, or more invalid values in the largest_niche (if they're the same size) - if somehow both of those properties are identical, we'd pick tagged because LLVM will likely handle it better.

@RalfJung RalfJung added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2020
@Muirrum
Copy link
Member

Muirrum commented Jun 20, 2020

@rustbot modify labels to +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2020
@RalfJung
Copy link
Member

Assigning to @eddyb as it is just an experiment. r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned RalfJung Jun 28, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…sakis

Compare tagged/niche-filling layout and pick the best one

Finishes up rust-lang#71045, and so fixes rust-lang#63866.

cc @eddyb
r? @nikomatsakis (since @eddyb wrote the first commit)
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…sakis

Compare tagged/niche-filling layout and pick the best one

Finishes up rust-lang#71045, and so fixes rust-lang#63866.

cc @eddyb
r? @nikomatsakis (since @eddyb wrote the first commit)
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…sakis

Compare tagged/niche-filling layout and pick the best one

Finishes up rust-lang#71045, and so fixes rust-lang#63866.

cc @eddyb
r? @nikomatsakis (since @eddyb wrote the first commit)
@eddyb
Copy link
Member Author

eddyb commented Jul 18, 2020

Thanks to @erikdesjardins, this was finished as #74069.

@eddyb eddyb closed this Jul 18, 2020
rust-timer added a commit to rust-timer/rust that referenced this pull request Jul 21, 2020
Original message:
Rollup merge of rust-lang#74069 - erikdesjardins:bad-niche, r=nikomatsakis

Compare tagged/niche-filling layout and pick the best one

Finishes up rust-lang#71045, and so fixes rust-lang#63866.

cc @eddyb
r? @nikomatsakis (since @eddyb wrote the first commit)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants