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.

Sign up for GitHub

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

Jump to bottom

Inline CStr::from_ptr #90007

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Inline CStr::from_ptr #90007

merged 1 commit into from
Oct 22, 2021

Conversation

KamilaBorowska
Copy link
Contributor

@KamilaBorowska KamilaBorowska commented Oct 18, 2021 •
edited
Loading

Inlining this function is valuable, as it allows LLVM to apply strlen-specific optimizations without having to enable LTO.

For instance, the following function:

pub fn f(p: *const c_char) -> Option<u8> {
    unsafe { CStr::from_ptr(p) }.to_bytes().get(0).copied()
}

Looks like this if CStr::from_ptr is allowed to be inlined.

before:
        push    rax
        call    qword ptr [rip + std::ffi::c_str::CStr::from_ptr@GOTPCREL]
        mov     rcx, rax
        cmp     rdx, 1
        sete    dl
        test    rax, rax
        sete    al
        or      al, dl
        jne     .LBB1_2
        mov     dl, byte ptr [rcx]
.LBB1_2:
        xor     al, 1
        pop     rcx
        ret

after:
        mov     dl, byte ptr [rdi]
        test    dl, dl
        setne   al
        ret

Note that optimization turned this from O(N) to O(1) in terms of performance, as LLVM knows that it doesn't really need to call strlen to determine whether a string is empty or not.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Oct 18, 2021
@kennytm
Copy link
Member

kennytm commented Oct 20, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2021

📌 Commit 86c309c has been approved by kennytm

@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 Oct 20, 2021
@matthiaskrgr
Copy link
Member

@bors rollup=never

@bors
Copy link
Contributor

bors commented Oct 21, 2021

⌛ Testing commit 86c309c with merge 145b4ae1789429e81b02f00393820ad7d32ab5cc...

@bors
Copy link
Contributor

bors commented Oct 21, 2021

💥 Test timed out

@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 Oct 21, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@kennytm
Copy link
Member

kennytm commented Oct 21, 2021

😕 Either incomplete log or networking issue.

https://github.com/rust-lang-ci/rust/runs/3962928502?check_suite_focus=true

@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 Oct 21, 2021
@bors
Copy link
Contributor

bors commented Oct 22, 2021

⌛ Testing commit 86c309c with merge 514b387...

@bors
Copy link
Contributor

bors commented Oct 22, 2021

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 514b387 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 22, 2021
@bors bors merged commit 514b387 into rust-lang:master Oct 22, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 22, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (514b387): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

@KamilaBorowska KamilaBorowska mentioned this pull request Aug 17, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 18, 2023
@matthiaskrgr
Rollup merge of rust-lang#114950 - xfix:inline-cstr-from-ptr, r=cuviper
1d05815 
Inline strlen_rt in CStr::from_ptr

This enables LLVM to optimize this function as if it was strlen (LLVM knows what it does, and can avoid calling it in certain situations) without having to enable std-aware LTO. This is essentially doing what rust-lang#90007 did, except updated for this function being `const`.

Pretty sure it's safe to roll-up, considering last time I did make this change it didn't affect performance (`CStr::from_ptr` isn't really used all that often in Rust code that is checked by rust-perf).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 19, 2023
@matthiaskrgr
Rollup merge of rust-lang#114950 - xfix:inline-cstr-from-ptr, r=cuviper
fbaf434 
Inline strlen_rt in CStr::from_ptr

This enables LLVM to optimize this function as if it was strlen (LLVM knows what it does, and can avoid calling it in certain situations) without having to enable std-aware LTO. This is essentially doing what rust-lang#90007 did, except updated for this function being `const`.

Pretty sure it's safe to roll-up, considering last time I did make this change it didn't affect performance (`CStr::from_ptr` isn't really used all that often in Rust code that is checked by rust-perf).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 19, 2023
@matthiaskrgr
Rollup merge of rust-lang#114950 - xfix:inline-cstr-from-ptr, r=cuviper
d98c7b3 
Inline strlen_rt in CStr::from_ptr

This enables LLVM to optimize this function as if it was strlen (LLVM knows what it does, and can avoid calling it in certain situations) without having to enable std-aware LTO. This is essentially doing what rust-lang#90007 did, except updated for this function being `const`.

Pretty sure it's safe to roll-up, considering last time I did make this change it didn't affect performance (`CStr::from_ptr` isn't really used all that often in Rust code that is checked by rust-perf).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 19, 2023
@matthiaskrgr
Rollup merge of rust-lang#114950 - xfix:inline-cstr-from-ptr, r=cuviper
a8d4e2a 
Inline strlen_rt in CStr::from_ptr

This enables LLVM to optimize this function as if it was strlen (LLVM knows what it does, and can avoid calling it in certain situations) without having to enable std-aware LTO. This is essentially doing what rust-lang#90007 did, except updated for this function being `const`.

Pretty sure it's safe to roll-up, considering last time I did make this change it didn't affect performance (`CStr::from_ptr` isn't really used all that often in Rust code that is checked by rust-perf).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 19, 2023
@matthiaskrgr
Rollup merge of rust-lang#114950 - xfix:inline-cstr-from-ptr, r=cuviper
cad8f8c 
Inline strlen_rt in CStr::from_ptr

This enables LLVM to optimize this function as if it was strlen (LLVM knows what it does, and can avoid calling it in certain situations) without having to enable std-aware LTO. This is essentially doing what rust-lang#90007 did, except updated for this function being `const`.

Pretty sure it's safe to roll-up, considering last time I did make this change it didn't affect performance (`CStr::from_ptr` isn't really used all that often in Rust code that is checked by rust-perf).
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Oct 17, 2023
@matthiaskrgr
Rollup merge of #114950 - xfix:inline-cstr-from-ptr, r=cuviper
bb27c07 
Inline strlen_rt in CStr::from_ptr

This enables LLVM to optimize this function as if it was strlen (LLVM knows what it does, and can avoid calling it in certain situations) without having to enable std-aware LTO. This is essentially doing what rust-lang/rust#90007 did, except updated for this function being `const`.

Pretty sure it's safe to roll-up, considering last time I did make this change it didn't affect performance (`CStr::from_ptr` isn't really used all that often in Rust code that is checked by rust-perf).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@kennytm kennytm

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.
8 participants
@KamilaBorowska @rust-highfive @kennytm @bors @matthiaskrgr @rust-log-analyzer @rust-timer @rustbot