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

Cow<[T]> layout forces unnecessary branching #117763

Closed
Jules-Bertholet opened this issue Nov 9, 2023 · 2 comments
Closed

Cow<[T]> layout forces unnecessary branching #117763

Jules-Bertholet opened this issue Nov 9, 2023 · 2 comments
Labels
A-layout Area: Memory layout of types C-bug Category: This is a bug. I-heavy Issue: Problems and improvements with respect to binary size of generated code. I-slow Issue: Problems and improvements with respect to performance of generated code. 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.

Comments

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Nov 9, 2023

I tried this code (Godbolt):

pub fn test<'a>(cow: &'a std::borrow::Cow<'a, [u8]>) -> &'a [u8] {
    &*cow
}

I expected to see this happen: The generated assembly is branchless. The pointer and length are at the same offset in both variants of the Cow.

Instead, this happened: There is a branch in the generated assembly. The pointer to the slice is at a different offset in the Owned and Borrowed variants of the Cow.


The ideal layout for Cow<[T]> (and Cow<str>) would look like this:

If `T` is not a ZST:
┌──────────┬──────────┬────────────────────────────────────────────────────┐
│ pointer  │ length   │ capacity if `Owned`, or `usize::MAX` if `Borrowed` │
└──────────┴──────────┴────────────────────────────────────────────────────┘

If `T` is a ZST:
┌──────────┬──────────┬────────────────────────────────────────┐
│ pointer  │ length   │ boolean flag for `Owned` vs `Borrowed` │
└──────────┴──────────┴────────────────────────────────────────┘

The present layout looks like this:

`Owned` variant:
┌──────────┬──────────┬──────────┐
│ pointer  │ capacity │ length   │
└──────────┴──────────┴──────────┘

`Borrowed` variant:
┌──────────┬──────────┬──────────┐
│ 0x0      │ pointer  │ length   │
└──────────┴──────────┴──────────┘

Because of this non-optimal layout, every access to the contained pointer requires a branch on the enum variant.

(Fixing #45431 is a prerequisite to fixing this)

Meta

rustc --version --verbose:

rustc 1.75.0-nightly (189d6c71f 2023-11-06)
binary: rustc
commit-hash: 189d6c71f3bb6c52113b5639a80839791974fd22
commit-date: 2023-11-06
host: x86_64-unknown-linux-gnu
release: 1.75.0-nightly
LLVM version: 17.0.4

@rustbot label A-layout I-heavy I-slow T-compiler T-libs

@Jules-Bertholet Jules-Bertholet added the C-bug Category: This is a bug. label Nov 9, 2023
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-layout Area: Memory layout of types I-heavy Issue: Problems and improvements with respect to binary size of generated code. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Nov 9, 2023
@Jules-Bertholet Jules-Bertholet changed the title Cow layout not ideal for slices and str Cow layout not optimal for slices and str Nov 9, 2023
@Jules-Bertholet Jules-Bertholet changed the title Cow layout not optimal for slices and str Cow<[T]> layout forces unnecessary branching Nov 9, 2023
@rustbot rustbot added 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. labels Nov 9, 2023
@the8472
Copy link
Member

the8472 commented Nov 9, 2023

It wouldn't solve #45431, but #106790 would add a larger niche to RawVec which could be used for the flag. Though that might actually turn it into ((cap, ptr), len)... which should also work for this purpose because then the flag can be stored in the first field before the borrowed payload.

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 10, 2023
@DaniPopes
Copy link
Contributor

DaniPopes commented Dec 29, 2023

Looks like this is fixed with #106790 in the latest nightly (Godbolt link in OP):

example::test:
        mov     rax, qword ptr [rdi + 8]
        mov     rdx, qword ptr [rdi + 16]
        ret

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Area: Memory layout of types C-bug Category: This is a bug. I-heavy Issue: Problems and improvements with respect to binary size of generated code. I-slow Issue: Problems and improvements with respect to performance of generated code. 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

No branches or pull requests

5 participants