single_char_pattern lint suggests performance regression for strip_prefix/suffix

[twanvl]

Summary

The lint single_char_pattern suggest replacing a string with a char constant, but this actually results in significantly worse performance for wide Unicode characters in strip_prefix.

In some sense this is a rust standard library issue, but clippy shouldn't suggest this as an improvement and say "Performing these methods using a char is faster than using a str." when the opposite is true.

This seems to be the case for strip_prefix and strip_suffix. For other methods like split, char is always faster than str.

Simplest fix: remove strip_prefix and strip_suffix from the list of functions to check in this lint.

Lint Name

single_char_pattern

Reproducer

Here is a benchmark:

use criterion::{black_box, criterion_group, criterion_main, Criterion};
pub fn criterion_benchmark(c: &mut Criterion) {
  c.bench_function("strip_prefix str  ascii",   |b| b.iter(|| black_box("hello world").strip_prefix("h")));
  c.bench_function("strip_prefix char ascii",   |b| b.iter(|| black_box("hello world").strip_prefix('h')));
  c.bench_function("strip_prefix str  unicode", |b| b.iter(|| black_box("おはよう、世界").strip_prefix("お")));
  c.bench_function("strip_prefix char unicode", |b| b.iter(|| black_box("おはよう、世界").strip_prefix('お')));
}
criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);

For which clippy reports:

warning: single-character string constant used as pattern
 --> src\main.rs:4:101
  |
4 |   c.bench_function("strip_prefix str  ascii",   |b| b.iter(|| black_box("hello world").strip_prefix("h")));
  |                                                                                                     ^^^ help: try using a `char` instead: `'h'`
  |
  = note: `#[warn(clippy::single_char_pattern)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern

warning: single-character string constant used as pattern
 --> src\main.rs:6:97
  |
6 |   c.bench_function("strip_prefix str  unicode", |b| b.iter(|| black_box("おはよう、世界").strip_prefix("お")));
  |                                                                                                        ^^^^ help: try using a `char` instead: `'お'`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern

The documentation says "Performing these methods using a char is faster than using a str."

Benchmark output:

strip_prefix str  ascii time:   [4.3805 ns 4.4939 ns 4.6732 ns]
strip_prefix char ascii time:   [4.2060 ns 4.2173 ns 4.2305 ns]
strip_prefix str  unicode
                        time:   [4.7124 ns 4.7187 ns 4.7270 ns]
strip_prefix char unicode
                        time:   [11.643 ns 11.677 ns 11.715 ns]

The performance with a single character Unicode pattern is better with a string pattern than with a char pattern. So clippy shouldn't suggest this change.

Version

rustc 1.59.0-nightly (efec54529 2021-12-04)
binary: rustc
commit-hash: efec545293b9263be9edfb283a7aa66350b3acbf
commit-date: 2021-12-04
host: x86_64-pc-windows-msvc
release: 1.59.0-nightly
LLVM version: 13.0.0

Additional Labels

No response

[8573]

This seems to be the case for strip_prefix and strip_suffix. For other methods like split, char is always faster than str.

I'm not familiar with the implementation of the standard library, but I would guess that a relevant difference is that the str::strip_* methods use the Pattern::strip_* methods, and <char as Pattern>::strip_* appear to be implemented by converting the char to a &str and simply forwarding to <&str as Pattern>::strip_*.

[llogiq]

@rustbot claim

The real question is if we should lint the char-taking versions of these, and if so, under the same lint or under a different one?

[oxalica]

For other methods like split, char is always faster than str.

My intuition is that multi-byte char will generally perform worse than &str for all methods, since &str is UTF-8 encoded while char (a codepoint) need to encode to UTF-8 first before performing byte-wise searching algorithm. Thus I suggest to suppress these lints on non-ASCII patterns for any method that accepts both. At least the pro is debatable, not to mention using &str pattern in the first place is more immune to future length changes.

My test shows split also performs worse on unicode char than &str.

pub fn criterion_benchmark(c: &mut Criterion) {
    c.bench_function("split str  ascii", |b| {
        b.iter(|| black_box("hell o wor ld").split(" ").count())
    });
    c.bench_function("split char ascii", |b| {
        b.iter(|| black_box("hell o wor ld").split(' ').count())
    });
    c.bench_function("split str  unicode", |b| {
        b.iter(|| black_box("お、はよう、世、界").split("、").count())
    });
    c.bench_function("split char unicode", |b| {
        b.iter(|| black_box("お、はよう、世、界").split('、').count())
    });
}

Result:

split str  ascii        time:   [19.815 ns 19.833 ns 19.869 ns]
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) high mild
  4 (4.00%) high severe

split char ascii        time:   [14.065 ns 14.074 ns 14.085 ns]
Found 23 outliers among 100 measurements (23.00%)
  2 (2.00%) low mild
  1 (1.00%) high mild
  20 (20.00%) high severe

split str  unicode      time:   [22.552 ns 22.597 ns 22.671 ns]
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low mild
  4 (4.00%) high mild
  5 (5.00%) high severe

split char unicode      time:   [29.886 ns 30.019 ns 30.145 ns]
Found 17 outliers among 100 measurements (17.00%)
  2 (2.00%) high mild
  15 (15.00%) high severe