-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Use unrolled loop for searching NULL in [u16] on Windows #67705
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
a61835f
to
a963e37
Compare
r? @BurntSushi |
Can you provide performance comparisons on both |
I don't have windows machine so I cannot bench this. |
Hm. After that they used the unrolled version: https://bugzilla.mozilla.org/show_bug.cgi?id=1033959 |
Should I port the unrolled version here or keep this change? |
Hi. Could any Windows devs help benchmarking to push this forward? |
I'm going to nominate this for compiler team to see if we can get a windows dev to benchmark this. Personally I'm not sure what are policy in general should be on patches like this, I'm tempted to just close (especially as no one has specifically requested this change in an issue). |
I have a Windows box I can test on. What do I need to do to run the benchmark? |
As per last triaging meeting discussion, assigning this one to Wesley. r? @wesleywiser |
Removing nomination since this was already discussed in last triage meeting. |
Thanks for the interests. And sorry the late reply. I was away from my computer in those days. I created a repo for benchmarking at https://github.com/lzutao/rust-benches-wmemchr. |
@wesleywiser Please see the post above. |
I get
when trying to run your benchmark. |
For Details
|
To be clear, It's hard to explain but using exactly the same nightly on the same system, Details
|
I don't know why but for MSVC targets, the unrolled loop version is
|
@lzutao benchmarking |
When building Using incremental compilation can have a huge impact on micro-benchmarks like this due to CGU partitioning. |
@wesleywiser in my case it was official nightly Now on the CI, No idea whether that benchmark has some bug or what. |
I ported the unrolled version (godbolt link) to use
Is this an optimization bug or not? |
I took a look in We should either confirm that this does not pessimize 32-bit Windows or only use this implementation on 64-bit Windows falling back to the existing code on 32-bit Windows. |
I benched it against 32-bit Windows (for both gnu and msvc) on Github Actions.
|
Sorry for the delay on this! @bors r+ |
📌 Commit 89bc236 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
Rollup of 7 pull requests Successful merges: - rust-lang#67705 (Use unrolled loop for searching NULL in [u16] on Windows) - rust-lang#70367 (save/restore `pessimistic_yield` when entering bodies) - rust-lang#70822 (Don't lint for self-recursion when the function can diverge) - rust-lang#70868 (rustc_codegen_ssa: Refactor construction of linker arguments) - rust-lang#70896 (Implement Chain with Option fuses) - rust-lang#70916 (Support `#[track_caller]` on functions in `extern "Rust" { ... }`) - rust-lang#70918 (rustc_session: forbid lints override regardless of position) Failed merges: r? @ghost
☔ The latest upstream changes (presumably #70943) made this pull request unmergeable. Please resolve the merge conflicts. |
Remove some redundant parts from `unrolled_find_u16s` See each commit message for details. r? @wesleywiser from old PR rust-lang#67705 .
No description provided.