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

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

Merged
merged 3 commits into from Jul 18, 2020

Conversation

erikdesjardins
Copy link
Contributor

Finishes up #71045, and so fixes #63866.

cc @eddyb
r? @nikomatsakis (since @eddyb wrote the first commit)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2020
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after nits are fixed

src/test/ui/type-sizes.rs Show resolved Hide resolved
src/test/ui/type-sizes.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jul 17, 2020

☔ The latest upstream changes (presumably #74395) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 17, 2020

📌 Commit 3924672 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 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)
@eddyb
Copy link
Member

eddyb commented Jul 18, 2020

Thanks a lot! I've closed my original unfinished PR.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 18, 2020
…arth

Rollup of 11 pull requests

Successful merges:

 - rust-lang#72414 ( Add lazy initialization primitives to std)
 - rust-lang#74069 (Compare tagged/niche-filling layout and pick the best one)
 - rust-lang#74418 (ci: Set `shell: bash` as a default, remove duplicates)
 - rust-lang#74441 (bootstrap.py: patch RPATH on NixOS to handle the new zlib dependency.)
 - rust-lang#74444 (Add regression test for rust-lang#69414)
 - rust-lang#74448 (improper_ctypes_definitions: allow `Box`)
 - rust-lang#74449 (Test codegen of compare_exchange operations)
 - rust-lang#74450 (Fix `Safety` docs for `from_raw_parts_mut`)
 - rust-lang#74453 (Use intra-doc links in `str` and `BTreeSet`)
 - rust-lang#74457 (rustbuild: drop tool::should_install)
 - rust-lang#74464 (Use pathdiff crate)

Failed merges:

r? @ghost
@bors bors merged commit e775b4d into rust-lang:master Jul 18, 2020
@erikdesjardins erikdesjardins deleted the bad-niche branch July 18, 2020 16:15
@Mark-Simulacrum
Copy link
Member

@rust-timer make-pr-for e775b4d

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)
@Mark-Simulacrum
Copy link
Member

Benchmarked in #74592, it does appear like this was a 5-10% performance regression on a large variety of crates. We should probably revert this PR. I've posted a revert here: xxx

https://perf.rust-lang.org/compare.html?start=d3df8512d2c2afc6d2e7d8b5b951dd7f2ad77b02&end=cfade73820883adf654fe34fd6b0b03a99458a51

cc @nnethercote

@Mark-Simulacrum
Copy link
Member

This turned out to not be the source of any regressions, and indeed might be a mild improvement. It will be re-landed in #74802. See also #74716 (comment) and the next few comments for discussion on this topic.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2020
…ercote

Reland rust-lang#74069

Investigation in rust-lang#74716 has concluded that this PR is indeed not a regression (and in fact the rollup itself is not either).

This reverts the revert in rust-lang#74611.

r? @nnethercote cc @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Niche-filling layouts are picked over tagged ones even when detrimental.
6 participants