Skip to content

Slicing after [u8]::first performs unnessessary bound check #75268

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

Closed
youknowone opened this issue Aug 7, 2020 · 5 comments
Closed

Slicing after [u8]::first performs unnessessary bound check #75268

youknowone opened this issue Aug 7, 2020 · 5 comments
Assignees
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. 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.

Comments

@youknowone
Copy link
Contributor

Playground link: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=2e2d548aa3a7b89bfb074456d971450d

pub fn boundcheck(mut lit: &[u8]) -> Option<&[u8]> {
    if lit.first() == Some(&b'_') {
        lit = &lit[1..];
        // lit = unsafe { lit.get_unchecked(1..) };
    }
    Some(lit)
}

This slice always exists because of the branch, but it performs bound check.

Real case: RustPython/RustPython#2077

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. 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. labels Aug 7, 2020
@Frizi
Copy link

Frizi commented Aug 7, 2020

Interestingly, following code seems to be properly optimized

pub fn boundcheck(mut lit: &[u8]) -> Option<&[u8]> {
    if let Some(&b'_') = lit.first() {
        lit = &lit[1..];
    }
    Some(lit)
}

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Aug 7, 2020

Another way to write that today:

pub fn boundcheck(mut lit: &[u8]) -> Option<&[u8]> {
    if let [b'_', rem @ ..] = lit {
        lit = rem;
    }
    Some(lit)
}

Before Rust 1.42.0:

pub fn boundcheck(mut lit: &[u8]) -> Option<&[u8]> {
    if let Some((&b'_', rem)) = lit.split_first() {
        lit = rem;
    }
    Some(lit)
}

@nikic
Copy link
Contributor

nikic commented Aug 8, 2020

Based on a local test #74533 fixes this issue as well.

@nikic nikic self-assigned this Aug 8, 2020
@tesuji
Copy link
Contributor

tesuji commented Aug 9, 2020

Closed by #74533 .
The new asm generated by nightly (ceedf1d 2020-08-08):

check::boundcheck:
 mov     rax, rdi
 test    rsi, rsi
 je      .LBB0_1
 mov     rdx, rsi
 cmp     byte, ptr, [rax], 95
 jne     .LBB0_4
 add     rax, 1
 add     rdx, -1
.LBB0_4:
 ret
.LBB0_1:
 xor     edx, edx
 ret

@sfackler sfackler closed this as completed Aug 9, 2020
@youknowone
Copy link
Contributor Author

Nice to know this will be fixed. Thank you all.
And thanks for the advice @rodrimati1992

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. 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.
Projects
None yet
Development

No branches or pull requests

7 participants