Skip to content
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

Fix a layout possible miscalculation in alloc::RawVec #83706

Merged
merged 4 commits into from
Feb 23, 2022

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Mar 31, 2021

A layout miscalculation could happen in RawVec when used with a type whose size isn't a multiple of its alignment. I don't know if such type can exist in Rust, but the Layout API provides ways to manipulate such types. Anyway, it is better to calculate memory size in a consistent way.

@rust-highfive
Copy link
Contributor

r? @joshtriplett

(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 Mar 31, 2021
@bjorn3
Copy link
Member

bjorn3 commented Mar 31, 2021

I don't know if such type can exist in Rust

I don't think so.

but the Layout API provides ways to manipulate such types.

Makes sense.

Anyway, it is better to calculate memory size in a consistent way.

I found at least one other place that used * size_of::<T>() inside shrink. There may be others.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2021
@bstrie bstrie added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

This seems reasonable and I agree that we should also take a look at shrink:

let (ptr, layout) = if let Some(mem) = self.current_memory() { mem } else { return Ok(()) };
let new_size = amount * mem::size_of::<T>();
let ptr = unsafe {
let new_layout = Layout::from_size_align_unchecked(new_size, layout.align());
self.alloc.shrink(ptr, layout, new_layout).map_err(|_| TryReserveError::AllocError {
layout: new_layout,
non_exhaustive: (),
})?
};

r=me once it's addressed.

@crlf0710 crlf0710 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 Jul 17, 2021
@JohnCSimon
Copy link
Member

JohnCSimon commented Jul 31, 2021

Ping from triage:
@a1phyr

r=me once it's addressed.

Can you please address this

@a1phyr a1phyr force-pushed the fix_vec_layout_calculation branch from 117b686 to fe942c2 Compare August 2, 2021 09:37
@a1phyr
Copy link
Contributor Author

a1phyr commented Aug 2, 2021

Done, also fixed capacity_from_bytes that did the same assumption.

@bors
Copy link
Collaborator

bors commented Aug 7, 2021

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

@a1phyr a1phyr force-pushed the fix_vec_layout_calculation branch from fe942c2 to e7f1c8e Compare August 7, 2021 10:33
@a1phyr
Copy link
Contributor Author

a1phyr commented Aug 7, 2021

Rebased

@JohnTitor
Copy link
Member

Sorry for the delay!
r? @JohnTitor @bors r+

@bors
Copy link
Collaborator

bors commented Aug 7, 2021

📌 Commit 03498aa has been approved by JohnTitor

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 7, 2021
@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 Feb 22, 2022
@bors
Copy link
Collaborator

bors commented Feb 22, 2022

⌛ Testing commit 5376317 with merge ed9196e48a1c25c95588058b775e6c71cd47eb83...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.......... (50/60)
.......... (60/60)


/checkout/src/test/rustdoc-gui/search-result-description.goml An exception occured: Failed to launch the browser process!
Inconsistency detected by ld.so: dl-tls.c: 493: _dl_allocate_tls_init: Assertion `listp->slotinfo[cnt].gen <= GL(dl_tls_generation)' failed!


TROUBLESHOOTING: https://github.com/puppeteer/puppeteer/blob/master/docs/troubleshooting.md
== STACKTRACE ==
Error
Error
    at innerRunTestCode (/node-v14.4.0-linux-x64/lib/node_modules/browser-ui-test/src/index.js:468:16)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

@bors
Copy link
Collaborator

bors commented Feb 22, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 22, 2022
@JohnTitor
Copy link
Member

Uhm, another spurious failure came up... @bors retry

@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 Feb 22, 2022
@bors
Copy link
Collaborator

bors commented Feb 22, 2022

⌛ Testing commit 5376317 with merge 5bd1ec3...

@bors
Copy link
Collaborator

bors commented Feb 23, 2022

☀️ Test successful - checks-actions
Approved by: JohnTitor
Pushing 5bd1ec3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 23, 2022
@bors bors merged commit 5bd1ec3 into rust-lang:master Feb 23, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 23, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5bd1ec3): comparison url.

Summary: This benchmark run shows 54 relevant regressions 😿 to instruction counts.

  • Average relevant regression: 0.5%
  • Largest regression in instruction counts: 1.2% on incr-unchanged builds of deeply-nested-async debug

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Feb 23, 2022
@rylev
Copy link
Member

rylev commented Feb 24, 2022

@JohnTitor @a1phyr the perf regressions are somewhat small but still there are enough of them to warrant a check in on what's going on. I assume a small perf regression would be expected since we're now strictly doing more (e.g., checked multiplication), but is the magnitude of the regression here expected?

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2023
simplify layout calculations in rawvec

The use of `Layout::array` was introduced in rust-lang#83706 which lead to a [perf regression](rust-lang#83706 (comment)).

This PR basically reverts that change since rust currently only supports stride == size types, but to be on the safe side it leaves a const-assert there to make sure this gets caught if those assumptions ever change.
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Feb 11, 2023
simplify layout calculations in rawvec

The use of `Layout::array` was introduced in #83706 which lead to a [perf regression](rust-lang/rust#83706 (comment)).

This PR basically reverts that change since rust currently only supports stride == size types, but to be on the safe side it leaves a const-assert there to make sure this gets caught if those assumptions ever change.
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request May 31, 2023
simplify layout calculations in rawvec

The use of `Layout::array` was introduced in #83706 which lead to a [perf regression](rust-lang/rust#83706 (comment)).

This PR basically reverts that change since rust currently only supports stride == size types, but to be on the safe side it leaves a const-assert there to make sure this gets caught if those assumptions ever change.
@a1phyr a1phyr deleted the fix_vec_layout_calculation branch March 12, 2024 17:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.