-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Delete architecture-specific memchr code in std::sys #114016
Conversation
r? @m-ou-se (rustbot has picked a reviewer for you, use r? to override) |
Hmm, I'm assuming this isn't going to affect anything but I would like to see the perf queue say "yes" first. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Delete architecture-specific memchr code in std::sys Currently all architecture-specific memchr code is only used in `std::io`. Most of the actual `memchr` capacity exposed to the user through the slice API is instead implemented in `core::slice::memchr`. Hence this commit deletes `memchr` from `std::sys[_common]` and replace calls to it by calls to `core::slice::memchr` functions. This deletes `(r)memchr` from the list of symbols linked to libc. The interest of putting architecture specific code back in core is linked to the discussion to be had in rust-lang#113654
☀️ Try build successful - checks-actions |
1 similar comment
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (3df3b2c): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 675.106s -> 674.558s (-0.08%) |
Hm. Sure. Don't know whether the other changes should be done, but the shocking lack of runtime perf changes feels motivating. @bors r+ |
…bilee Delete architecture-specific memchr code in std::sys Currently all architecture-specific memchr code is only used in `std::io`. Most of the actual `memchr` capacity exposed to the user through the slice API is instead implemented in `core::slice::memchr`. Hence this commit deletes `memchr` from `std::sys[_common]` and replace calls to it by calls to `core::slice::memchr` functions. This deletes `(r)memchr` from the list of symbols linked to libc. The interest of putting architecture specific code back in core is linked to the discussion to be had in rust-lang#113654
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
uh? |
8ddbdb2
to
8473ea8
Compare
I'm going to just hand-audit this for every supported platform before I approve this again. Might be a few days before I get around to it. |
Ok. I'm on break for the next two weeks anyway so no rush. |
☔ The latest upstream changes (presumably #117285) made this pull request unmergeable. Please resolve the merge conflicts. |
8473ea8
to
c51854e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some unresolved imports and missing modules on some targets:
- Hermit (x86_64-unknown-hermit)
- SOLID (aarch64-kmc-solid_asp3)
- TeeOS (aarch64-unknown-teeos)
- UEFI still has a
memchr
module (x86_64-unknown-uefi) - ZKVM does too (it doesn't currently build anyway)
I'd recommend searching all of std
for memchr
, checking each use, and then running x check library/std --target [TARGET]
for each of the targets above, they should be representative.
☔ The latest upstream changes (presumably #121177) made this pull request unmergeable. Please resolve the merge conflicts. |
c51854e
to
ca6b355
Compare
This PR modifies If appropriate, please update |
Currently all architecture-specific memchr code is only used in `std::io`. Most of the actual `memchr` capacity exposed to the user through the slice API is instead implemented in core::slice::memchr. Hence this commit deletes memchr from std::sys[_common] and replace calls to it by calls to core::slice::memchr functions. This deletes (r)memchr from the list of symbols linked to libc.
ca6b355
to
88ac7ac
Compare
@rustbot label -T-bootstrap |
Thanks @joboet, it should hopefully be good know. |
Thank you everyone! Sorry for not getting back to you on answering your question on Zulip, the answer was a somewhat inane "manually comb through everything and run this code on the machines I have available". @bors r+ |
@bors rollup=never |
☀️ Test successful - checks-actions |
Finished benchmarking commit (6f435eb): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 651.937s -> 650.561s (-0.21%) |
The regression doesn’t look real, syn-opt was somehow less noisy than usual recently, and seems to be back to its usual behavior. |
Currently all architecture-specific memchr code is only used in
std::io
. Most of the actualmemchr
capacity exposed to the user through the slice API is instead implemented incore::slice::memchr
.Hence this commit deletes
memchr
fromstd::sys[_common]
and replace calls to it by calls tocore::slice::memchr
functions. This deletes(r)memchr
from the list of symbols linked to libc.The interest of putting architecture specific code back in core is linked to the discussion to be had in #113654