impl Compare<&str> and Compare<&[u8]> for AsRef<[u8]>

[soulstompp]

I noticed that a handful of impls of Compare could be eliminated by handling AsRef<[u8]> more generically. I feel like the code is ever-so slightly more manageable this way. I wanted to open this issue to allow for discussion and will relate my pull request to this issue here shortly.

What I wasn't expecting was it seems to have helped out with performance across the majority of benchmarks and left everything else neutral. I assume the improvement is because &str (which I know causes some performance issues in some instances) is now being compared as &[u8] even when both sides of the &str comparisons.

These are the benchmark results I got after running against main and then my branch:

t not found, using plotters backend
http/complete/291       time:   [628.45 ns 632.36 ns 637.42 ns]
                        thrpt:  [435.38 MiB/s 438.86 MiB/s 441.59 MiB/s]
                 change:
                        time:   [-3.9294% -3.5841% -3.2019%] (p = 0.00 < 0.05)
                        thrpt:  [+3.3079% +3.7173% +4.0901%]
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe
http/streaming/291      time:   [739.42 ns 739.92 ns 740.46 ns]
                        thrpt:  [374.79 MiB/s 375.07 MiB/s 375.32 MiB/s]
                 change:
                        time:   [-5.7336% -5.3313% -4.9403%] (p = 0.00 < 0.05)
                        thrpt:  [+5.1970% +5.6315% +6.0823%]
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  3 (3.00%) high mild
  3 (3.00%) high severe

     Running examples/ini/bench.rs (target/release/deps/ini-43021e6b6bb897e2)
Gnuplot not found, using plotters backend
ini/bytes/110           time:   [421.09 ns 422.31 ns 423.56 ns]
                        thrpt:  [247.67 MiB/s 248.41 MiB/s 249.13 MiB/s]
                 change:
                        time:   [-11.524% -10.950% -10.450%] (p = 0.00 < 0.05)
                        thrpt:  [+11.669% +12.296% +13.025%]
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild
ini/str/110             time:   [458.08 ns 459.91 ns 461.95 ns]
                        thrpt:  [227.09 MiB/s 228.10 MiB/s 229.01 MiB/s]
                 change:
                        time:   [-13.522% -12.544% -11.641%] (p = 0.00 < 0.05)
                        thrpt:  [+13.174% +14.343% +15.636%]
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

ini keys and values/bytes/45
                        time:   [59.519 ns 59.655 ns 59.823 ns]
                        thrpt:  [717.37 MiB/s 719.39 MiB/s 721.03 MiB/s]
                 change:
                        time:   [-4.2934% -3.7890% -3.3251%] (p = 0.00 < 0.05)
                        thrpt:  [+3.4395% +3.9382% +4.4860%]
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe

ini key value/bytes/18  time:   [35.245 ns 35.297 ns 35.354 ns]
                        thrpt:  [485.55 MiB/s 486.33 MiB/s 487.06 MiB/s]
                 change:
                        time:   [-0.1705% +0.3476% +0.7708%] (p = 0.15 > 0.05)
                        thrpt:  [-0.7649% -0.3464% +0.1708%]
                        No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
  5 (5.00%) high mild
  4 (4.00%) high severe

     Running benches/iter.rs (target/release/deps/iter-13759c3d58f4262a)
Gnuplot not found, using plotters backend
iter/iterate/contiguous time:   [133.07 ns 133.25 ns 133.45 ns]
                        thrpt:  [1.8842 GiB/s 1.8872 GiB/s 1.8896 GiB/s]
                 change:
                        time:   [-1.3277% -0.4614% +0.0950%] (p = 0.26 > 0.05)
                        thrpt:  [-0.0949% +0.4636% +1.3456%]
                        No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
  5 (5.00%) high mild
  5 (5.00%) high severe
iter/next_token/contiguous
                        time:   [133.18 ns 133.36 ns 133.55 ns]
                        thrpt:  [1.8829 GiB/s 1.8855 GiB/s 1.8881 GiB/s]
                 change:
                        time:   [-0.3923% -0.0469% +0.3276%] (p = 0.80 > 0.05)
                        thrpt:  [-0.3265% +0.0470% +0.3939%]
                        No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) high mild
  5 (5.00%) high severe
iter/opt(one_of)/contiguous
                        time:   [112.61 ns 113.09 ns 113.79 ns]
                        thrpt:  [2.2099 GiB/s 2.2235 GiB/s 2.2330 GiB/s]
                 change:
                        time:   [-0.6142% -0.0814% +0.4932%] (p = 0.78 > 0.05)
                        thrpt:  [-0.4908% +0.0814% +0.6180%]
                        No change in performance detected.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) low mild
  7 (7.00%) high severe
iter/take_while/contiguous
                        time:   [101.38 ns 101.47 ns 101.57 ns]
                        thrpt:  [2.4757 GiB/s 2.4781 GiB/s 2.4804 GiB/s]
                 change:
                        time:   [-2.0523% -1.4994% -1.0073%] (p = 0.00 < 0.05)
                        thrpt:  [+1.0176% +1.5222% +2.0953%]
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe
iter/repeat/contiguous  time:   [104.46 ns 104.63 ns 104.88 ns]
                        thrpt:  [2.3976 GiB/s 2.4034 GiB/s 2.4073 GiB/s]
                 change:
                        time:   [-1.1494% -0.6905% -0.2728%] (p = 0.00 < 0.05)
                        thrpt:  [+0.2736% +0.6953% +1.1627%]
                        Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) high mild
  3 (3.00%) high severe
iter/iterate/interleaved
                        time:   [136.95 ns 137.12 ns 137.30 ns]
                        thrpt:  [1.8789 GiB/s 1.8815 GiB/s 1.8837 GiB/s]
                 change:
                        time:   [-0.2319% +0.1002% +0.3844%] (p = 0.55 > 0.05)
                        thrpt:  [-0.3829% -0.1001% +0.2324%]
                        No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
  1 (1.00%) low mild
  6 (6.00%) high mild
  3 (3.00%) high severe
iter/next_token/interleaved
                        time:   [137.01 ns 137.23 ns 137.46 ns]
                        thrpt:  [1.8767 GiB/s 1.8799 GiB/s 1.8828 GiB/s]
                 change:
                        time:   [+0.0695% +0.3921% +0.7075%] (p = 0.01 < 0.05)
                        thrpt:  [-0.7025% -0.3905% -0.0695%]
                        Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) high mild
  3 (3.00%) high severe
iter/opt(one_of)/interleaved
                        time:   [352.84 ns 353.25 ns 353.71 ns]
                        thrpt:  [746.84 MiB/s 747.82 MiB/s 748.70 MiB/s]
                 change:
                        time:   [-15.495% -12.425% -9.3695%] (p = 0.00 < 0.05)
                        thrpt:  [+10.338% +14.188% +18.336%]
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  1 (1.00%) low mild
  5 (5.00%) high mild
  4 (4.00%) high severe
iter/take_while/interleaved
                        time:   [117.49 ns 117.74 ns 117.99 ns]
                        thrpt:  [2.1865 GiB/s 2.1911 GiB/s 2.1957 GiB/s]
                 change:
                        time:   [-12.575% -10.890% -9.2474%] (p = 0.00 < 0.05)
                        thrpt:  [+10.190% +12.220% +14.384%]
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe
iter/repeat/interleaved time:   [341.48 ns 341.84 ns 342.23 ns]
                        thrpt:  [771.91 MiB/s 772.78 MiB/s 773.60 MiB/s]
                 change:
                        time:   [-0.2091% +0.0235% +0.2486%] (p = 0.85 > 0.05)
                        thrpt:  [-0.2480% -0.0235% +0.2096%]
                        No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) high mild
  4 (4.00%) high severe
Benchmarking iter/iterate/canada: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.7s, enable flat sampling, or reduce sample count to 60.
iter/iterate/canada     time:   [1.0994 ms 1.1007 ms 1.1022 ms]
                        thrpt:  [1.9021 GiB/s 1.9047 GiB/s 1.9070 GiB/s]
                 change:
                        time:   [-3.7393% -2.7848% -1.8780%] (p = 0.00 < 0.05)
                        thrpt:  [+1.9139% +2.8645% +3.8846%]
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) high mild
  4 (4.00%) high severe
Benchmarking iter/next_token/canada: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.7s, enable flat sampling, or reduce sample count to 60.
iter/next_token/canada  time:   [1.0987 ms 1.1036 ms 1.1122 ms]
                        thrpt:  [1.8849 GiB/s 1.8996 GiB/s 1.9081 GiB/s]
                 change:
                        time:   [-2.2849% -1.7645% -1.2361%] (p = 0.00 < 0.05)
                        thrpt:  [+1.2515% +1.7962% +2.3383%]
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) high mild
  4 (4.00%) high severe
iter/opt(one_of)/canada time:   [3.2718 ms 3.2757 ms 3.2805 ms]
                        thrpt:  [654.41 MiB/s 655.36 MiB/s 656.15 MiB/s]
                 change:
                        time:   [-1.9359% -1.6823% -1.4153%] (p = 0.00 < 0.05)
                        thrpt:  [+1.4357% +1.7111% +1.9741%]
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe
Benchmarking iter/take_while/canada: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.8s, enable flat sampling, or reduce sample count to 60.
iter/take_while/canada  time:   [1.1089 ms 1.1108 ms 1.1126 ms]
                        thrpt:  [1.8842 GiB/s 1.8874 GiB/s 1.8905 GiB/s]
                 change:
                        time:   [-5.5130% -4.4118% -3.4732%] (p = 0.00 < 0.05)
                        thrpt:  [+3.5981% +4.6154% +5.8347%]
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe
iter/repeat/canada      time:   [3.1809 ms 3.1845 ms 3.1884 ms]
                        thrpt:  [673.31 MiB/s 674.14 MiB/s 674.88 MiB/s]
                 change:
                        time:   [-4.0904% -3.4705% -2.8934%] (p = 0.00 < 0.05)
                        thrpt:  [+2.9796% +3.5953% +4.2648%]
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

     Running examples/json/bench.rs (target/release/deps/json-14206d59e04faf0a)
Gnuplot not found, using plotters backend
json/unit/small         time:   [1.2023 µs 1.2045 µs 1.2063 µs]
                        thrpt:  [79.849 MiB/s 79.970 MiB/s 80.114 MiB/s]
                 change:
                        time:   [-6.5962% -5.7851% -5.0379%] (p = 0.00 < 0.05)
                        thrpt:  [+5.3052% +6.1403% +7.0621%]
                        Performance has improved.
Found 22 outliers among 100 measurements (22.00%)
  11 (11.00%) low severe
  2 (2.00%) low mild
  4 (4.00%) high mild
  5 (5.00%) high severe
json/context/small      time:   [1.7267 µs 1.7314 µs 1.7362 µs]
                        thrpt:  [55.479 MiB/s 55.633 MiB/s 55.785 MiB/s]
                 change:
                        time:   [-8.6067% -8.1585% -7.7117%] (p = 0.00 < 0.05)
                        thrpt:  [+8.3561% +8.8833% +9.4172%]
                        Performance has improved.
Found 27 outliers among 100 measurements (27.00%)
  13 (13.00%) low severe
  1 (1.00%) low mild
  13 (13.00%) high severe
json/dispatch/small     time:   [1.2345 µs 1.2358 µs 1.2373 µs]
                        thrpt:  [77.845 MiB/s 77.942 MiB/s 78.023 MiB/s]
                 change:
                        time:   [-4.2072% -3.3343% -2.4249%] (p = 0.00 < 0.05)
                        thrpt:  [+2.4852% +3.4493% +4.3920%]
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild
  3 (3.00%) high severe
json/streaming/small    time:   [1.6973 µs 1.7010 µs 1.7049 µs]
                        thrpt:  [56.496 MiB/s 56.627 MiB/s 56.751 MiB/s]
                 change:
                        time:   [-1.5899% -0.5110% +0.1438%] (p = 0.34 > 0.05)
                        thrpt:  [-0.1436% +0.5136% +1.6156%]
                        No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
  4 (4.00%) high mild
  1 (1.00%) high severe
json/unit/canada        time:   [16.986 ms 17.023 ms 17.061 ms]
                        thrpt:  [125.83 MiB/s 126.11 MiB/s 126.39 MiB/s]
                 change:
                        time:   [-3.2889% -2.7618% -2.2648%] (p = 0.00 < 0.05)
                        thrpt:  [+2.3173% +2.8403% +3.4008%]
                        Performance has improved.
json/context/canada     time:   [26.574 ms 26.636 ms 26.698 ms]
                        thrpt:  [80.409 MiB/s 80.597 MiB/s 80.785 MiB/s]
                 change:
                        time:   [-8.2500% -7.4629% -6.7005%] (p = 0.00 < 0.05)
                        thrpt:  [+7.1817% +8.0647% +8.9918%]
                        Performance has improved.
json/dispatch/canada    time:   [15.919 ms 15.948 ms 15.976 ms]
                        thrpt:  [134.38 MiB/s 134.61 MiB/s 134.85 MiB/s]
                 change:
                        time:   [-9.8712% -9.1336% -8.4187%] (p = 0.00 < 0.05)
                        thrpt:  [+9.1925% +10.052% +10.952%]
                        Performance has improved.
Found 19 outliers among 100 measurements (19.00%)
  4 (4.00%) low severe
  14 (14.00%) low mild
  1 (1.00%) high severe
json/streaming/canada   time:   [26.497 ms 26.559 ms 26.623 ms]
                        thrpt:  [80.636 MiB/s 80.829 MiB/s 81.019 MiB/s]
                 change:
                        time:   [-10.598% -9.3461% -8.1311%] (p = 0.00 < 0.05)
                        thrpt:  [+8.8508% +10.310% +11.854%]
                        Performance has improved.

     Running benches/next_slice.rs (target/release/deps/next_slice-742fb1398930047a)
Gnuplot not found, using plotters backend
next_slice/char/ascii   time:   [403.39 ns 403.86 ns 404.38 ns]
                        change: [-6.7838% -5.7665% -4.8311%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  3 (3.00%) high mild
  4 (4.00%) high severe
next_slice/str/ascii    time:   [263.63 ns 264.02 ns 264.42 ns]
                        change: [-4.5333% -4.1305% -3.7264%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low mild
  3 (3.00%) high mild
  3 (3.00%) high severe
next_slice/one_of/ascii time:   [182.23 ns 182.37 ns 182.53 ns]
                        change: [-6.2562% -5.6089% -4.9403%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  3 (3.00%) high severe
next_slice/tag_char/ascii
                        time:   [435.74 ns 436.15 ns 436.54 ns]
                        change: [-3.9677% -3.4910% -3.0001%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) high mild
  4 (4.00%) high severe
next_slice/tag_str/ascii
                        time:   [260.64 ns 261.12 ns 261.69 ns]
                        change: [-5.1850% -4.5914% -3.9561%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  8 (8.00%) high severe
next_slice/char/utf8    time:   [614.19 ns 614.93 ns 615.69 ns]
                        change: [-6.7359% -5.9081% -5.1185%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  2 (2.00%) low mild
  5 (5.00%) high mild
  6 (6.00%) high severe
next_slice/str/utf8     time:   [429.61 ns 430.34 ns 431.31 ns]
                        change: [-4.3139% -3.4439% -2.6419%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  2 (2.00%) high mild
  7 (7.00%) high severe
next_slice/one_of/utf8  time:   [395.67 ns 396.16 ns 396.68 ns]
                        change: [-4.4335% -3.6710% -2.9199%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  2 (2.00%) high mild
  5 (5.00%) high severe
next_slice/tag_char/utf8
                        time:   [653.41 ns 653.83 ns 654.26 ns]
                        change: [-6.1019% -5.0645% -4.0484%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  2 (2.00%) high mild
  8 (8.00%) high severe
next_slice/tag_str/utf8 time:   [438.74 ns 445.30 ns 452.88 ns]
                        change: [-1.3223% -0.4780% +0.4420%] (p = 0.30 > 0.05)
                        No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
  1 (1.00%) high mild
  9 (9.00%) high severe

     Running benches/number.rs (target/release/deps/number-1d1a135d1aad0c2c)
Gnuplot not found, using plotters backend
number                  time:   [728.09 ps 731.75 ps 736.61 ps]
                        change: [-1.3439% -0.8011% -0.2353%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 13 outliers among 100 measurements (13.00%)
  6 (6.00%) high mild
  7 (7.00%) high severe

float_bytes result: Ok(([], -1.234e-12))
float bytes             time:   [35.153 ns 35.490 ns 35.874 ns]
                        change: [-5.5100% -4.8517% -4.2127%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) high mild
  6 (6.00%) high severe

std_float_bytes result: Ok(([45, 49, 46, 50, 51, 52, 69, 45, 49, 50], -1.234e-12))
std_float bytes         time:   [25.271 ns 25.321 ns 25.373 ns]
                        change: [-5.1116% -4.7089% -4.3535%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild

float_str result: Ok(("", -1.234e-12))
float str               time:   [35.252 ns 35.295 ns 35.339 ns]
                        change: [-1.3537% -0.8619% -0.3974%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  3 (3.00%) high severe

[epage]

Sorry for the delay; I'm still working to catch up to everything from the US holiays.

Compare<&[u8]> for AsRef<[u8]>

That would allow slicing of a &str along non-UTF-8 boundaries. Wherever possible, we try to limit the API so people don't run into bugs with that. I've even been considering doing a pass to see if it would be safe for us to assume we always have UTF-8 boundaries and to use unsafe (#115)

impl Compare<&str> and Compare<&[u8]> for AsRef<[u8]>

More generally,

• Making this change would be a breaking change because someone could have an impl Compare for a AsRef<[u8]> type and we'd at least need to wait for the next time we make a breaking change
• I am cautious about using blanket impls because they are limiting in what else you can then do with a trait, especially without specialization
• Even if we did this for now until we had a need for removing blanket impls, that would then be a breaking change and would block resolving that need until we make a breaking change.

What I wasn't expecting was it seems to have helped out with performance across the majority of benchmarks and left everything else neutral. I assume the improvement is because &str (which I know causes some performance issues in some instances) is now being compared as &[u8] even when both sides of the &str comparisons.

I wonder why that is. All of our comparisons are turned into byte comparisons already with #[inline(always)] which should remove any overhead of extra wrappers

winnow/src/stream/mod.rs

Lines 1045 to 1050 in 2566343

	impl<'b> Compare<&'b str> for &str {
	#[inline(always)]
	fn compare(&self, t: &'b str) -> CompareResult {
	self.as_bytes().compare(t.as_bytes())
	}
	}

Might be worth seeing what kind of changes could be made to the current implementation to reproduce this.

[epage]

I wonder why that is.

One difference is that #635 doesn't have #[inline] but its generic which makes it similar to #[inline] (but not quite).

[soulstompp]

I wonder why that is.

One difference is that #635 doesn't have #[inline] but its generic which makes it similar to #[inline] (but not quite).

That makes sense.

[soulstompp]

Sorry for the delay; I'm still working to catch up to everything from the US holiays.

Compare<&[u8]> for AsRef<[u8]>

That would allow slicing of a &str along non-UTF-8 boundaries. Wherever possible, we try to limit the API so people don't run into bugs with that. I've even been considering doing a pass to see if it would be safe for us to assume we always have UTF-8 boundaries and to use unsafe (#115)

impl Compare<&str> and Compare<&[u8]> for AsRef<[u8]>

I was afraid there might be some effects that I had overlooked, I hadn't considered non-UTF-8 boundaries.

More generally,

* Making this change would be a breaking change because someone could have an `impl Compare` for a `AsRef<[u8]>` type and we'd at least need to wait for the next time we make a breaking change

* I am cautious about using blanket impls because they are limiting in what else you can then do with a trait, especially without specialization

* Even if we did this for now until we had a need for removing blanket impls, that would then be a breaking change and would block resolving that need until we make a breaking change.

These are all really good points that could definitely be problematic. I am trying to learn to be more careful with my own projects as I (hopefully) grow a user-base, so I appreciate your taking the time to detail these possible gotchas that this could possibly cause for others.

[epage]

Going to go ahead and close for now but still interested in someone looking to see if we can get those performance benefits.

And if there is a reason for us to re-evaluate the request more generally, let us know!

[soulstompp]

I will try poking around and see if I can find where we might find these performance boosts. The notes from here and the benchmarks (which I will need to redo on the latest version) there should be enough information to get me started.