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

Remove ptr-int transmute in std::sync::mpsc #95621

Merged
merged 1 commit into from
Apr 10, 2022

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Apr 3, 2022

Since #95340 landed, Miri with -Zmiri-check-number-validity produces an error on the test suites of some crates which implement concurrency tools*, because it seems like such crates tend to use std::sync::mpsc in their tests. This fixes the problem by storing pointer bytes in a pointer.

* I have so far seen errors in the test suites of once_cell, parking_lot, and crossbeam-utils.
(just updating the list for fun, idk)
Also threadpool, async-lock, futures-timer, fragile, scoped_threadpool, procfs, slog-async, scheduled-thread-pool, tokio-threadpool, mac, futures-cpupool, ntest, actix, zbus, jsonrpc-client-transports, fail, libp2p-gossipsub, parity-send-wrapper, async-broadcast, libp2p-relay, http-client, mockito, simple-mutex, surf, pollster, and pulse. Then I turned the bot off.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(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 3, 2022
@saethlin saethlin force-pushed the remove-mpsc-transmute branch from d6e824e to 127309f Compare April 3, 2022 21:01
@saethlin saethlin force-pushed the remove-mpsc-transmute branch from 127309f to a696e0a Compare April 4, 2022 03:15
@saethlin
Copy link
Member Author

saethlin commented Apr 4, 2022

The previous CI run failed due to a 360 minute timeout. Interesting.

@yaahc yaahc added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 4, 2022
Since rust-lang#95340 landed, Miri with
-Zmiri-check-number-validity produces an error on the test suites of
some crates which implement concurrency tools, because it seems like
such crates tend to use std::sync::mpsc in their tests. This fixes the
problem by storing pointer bytes in a pointer.
@saethlin saethlin force-pushed the remove-mpsc-transmute branch from a696e0a to dec73f5 Compare April 9, 2022 03:28
@RalfJung
Copy link
Member

RalfJung commented Apr 9, 2022

The patch LGTM (it's mostly a straight-forward replacement of usize by a pointer type), and I checked that mpsc/tests.rs after this PR passes in Miri with strict provenance. So, I think we can land this.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 9, 2022

📌 Commit dec73f5 has been approved by RalfJung

@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 Apr 9, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 9, 2022
…alfJung

Remove ptr-int transmute in std::sync::mpsc

Since rust-lang#95340 landed, Miri with `-Zmiri-check-number-validity` produces an error on the test suites of some crates which implement concurrency tools<sup>*</sup>, because it seems like such crates tend to use `std::sync::mpsc` in their tests. This fixes the problem by storing pointer bytes in a pointer.

<sup>*</sup> I have so far seen errors in the test suites of `once_cell`, `parking_lot`, and `crossbeam-utils`.
(just updating the list for fun, idk)
Also `threadpool`, `async-lock`, `futures-timer`, `fragile`, `scoped_threadpool`, `procfs`, `slog-async`, `scheduled-thread-pool`, `tokio-threadpool`, `mac`, `futures-cpupool`, `ntest`, `actix`, `zbus`, `jsonrpc-client-transports`, `fail`, `libp2p-gossipsub`, `parity-send-wrapper`, `async-broadcast,` `libp2p-relay`, `http-client`, `mockito`, `simple-mutex`, `surf`, `pollster`, and `pulse`. Then I turned the bot off.
@bors
Copy link
Contributor

bors commented Apr 10, 2022

⌛ Testing commit dec73f5 with merge 7af9329...

@bors
Copy link
Contributor

bors commented Apr 10, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 7af9329 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 10, 2022
@bors bors merged commit 7af9329 into rust-lang:master Apr 10, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 10, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7af9329): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 1 6 0 0 1
mean2 0.5% 0.4% N/A N/A 0.5%
max 0.5% 0.5% N/A N/A 0.5%

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

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@rustbot rustbot added the perf-regression Performance regression. label Apr 10, 2022
@RalfJung RalfJung mentioned this pull request Apr 10, 2022
bors added a commit to rust-lang/miri that referenced this pull request Apr 10, 2022
rustup

rust-lang/rust#95621 made channels strict provenance compliant. :)
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Apr 12, 2022

Highly likely a real regression, but it's small enough that I'm not too worried, and seems to largely be due to increases in trait-related queries. This is probably due to increased complexity of standard library types making it a little more expensive to prove things like Sized, Send, etc.

May also be noise and paired with #95253 / https://perf.rust-lang.org/compare.html?start=7af93292c27cd8b4a14f0f35bcb4c7e7ca9c287a&end=32c26302620b2dbbe0a2291f1969bc0b1622ae59&stat=instructions:u, which reverted most of the regression here.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants