Inline the fast path of BitWriter::send_bits

[brian-pane]

The "equal" and "greater" branches yield the same result when total_bits == Self::BIT_BUF_SIZE, so this change combines them to eliminate one conditional branch whenever the bit_buffer is full.

Benchmark results before:

hyperfine -m 100 --warmup 10 "./target/release/examples/compress 1 rs silesia-small.tar"
Benchmark 1: ./target/release/examples/compress 1 rs silesia-small.tar
  Time (mean ± σ):      92.2 ms ±   1.2 ms    [User: 79.9 ms, System: 12.3 ms]
  Range (min … max):    90.1 ms …  96.6 ms    100 runs

and with this change applied:

hyperfine -m 100 --warmup 10 "./target/release/examples/compress 1 rs silesia-small.tar"
Benchmark 1: ./target/release/examples/compress 1 rs silesia-small.tar
  Time (mean ± σ):      91.3 ms ±   0.9 ms    [User: 79.0 ms, System: 12.2 ms]
  Range (min … max):    89.7 ms …  95.2 ms    100 runs

[folkertdev]

Hi,

For benchmarking it is crucial that you actually race the two implementations against each other: just comparing wall clock times is unreliable, especially when the times are so close together (and in fact their ranges overlap).

When I run this commit agains its base commit, I actually observe a regression

Benchmark 1 (65 runs): ./compress-baseline 1 rs silesia-small.tar
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          77.2ms ±  844us    76.2ms … 82.6ms          4 ( 6%)        0%
  peak_rss           26.7MB ± 64.3KB    26.6MB … 26.7MB          0 ( 0%)        0%
  cpu_cycles          283M  ± 2.02M      279M  …  292M           4 ( 6%)        0%
  instructions        633M  ±  277       633M  …  633M           0 ( 0%)        0%
  cache_references   20.0M  ±  162K     19.7M  … 20.7M           2 ( 3%)        0%
  cache_misses        466K  ± 67.0K      374K  …  735K           2 ( 3%)        0%
  branch_misses      3.06M  ± 7.05K     3.05M  … 3.09M           1 ( 2%)        0%
Benchmark 2 (64 runs): ./target/release/examples/blogpost-compress 1 rs silesia-small.tar
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          79.1ms ±  422us    78.5ms … 80.3ms          2 ( 3%)        💩+  2.5% ±  0.3%
  peak_rss           26.7MB ± 68.6KB    26.5MB … 26.7MB          0 ( 0%)          -  0.0% ±  0.1%
  cpu_cycles          292M  ± 1.31M      290M  …  297M           2 ( 3%)        💩+  3.3% ±  0.2%
  instructions        659M  ±  300       659M  …  659M           1 ( 2%)        💩+  4.1% ±  0.0%
  cache_references   19.9M  ±  190K     19.6M  … 20.5M           1 ( 2%)          -  0.8% ±  0.3%
  cache_misses        455K  ± 79.0K      353K  …  723K           1 ( 2%)          -  2.3% ±  5.4%
  branch_misses      3.11M  ± 2.09K     3.11M  … 3.12M           0 ( 0%)        💩+  1.6% ±  0.1%

(I'm using https://github.com/andrewrk/poop here)

The branch that you remove is not actually conditional in practice: it is very well-predicted. While it doesn't look like it, this code is actually fairly optimized and it's hard to do better (I've tried!).

So, I guess, see what you get when you run the two benchmarks alongside each other (hyperfine accepts multiple things to benchmark, you just need to copy the binary from before your change somewhere so you can use it after applying your change).

[brian-pane]

Thanks, I'll adopt the same benchmark method. In my initial test run using the same benchmark driver, I see an improvement in branch mispredictions, although it's offset by an increase in instructions executed:

poop "./compress-baseline 1 rs silesia-small.tar" "./target/release/examples/compress 1 rs silesia-small.tar" 
Benchmark 1 (55 runs): ./compress-baseline 1 rs silesia-small.tar
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          91.6ms ± 1.51ms    90.3ms …  101ms          4 ( 7%)        0%
  peak_rss           26.7MB ± 67.0KB    26.5MB … 26.7MB          0 ( 0%)        0%
  cpu_cycles          341M  ±  706K      340M  …  343M           0 ( 0%)        0%
  instructions        748M  ±  234       748M  …  748M           0 ( 0%)        0%
  cache_references    399K  ± 2.02K      396K  …  406K           3 ( 5%)        0%
  cache_misses        294K  ± 8.94K      266K  …  313K           6 (11%)        0%
  branch_misses      3.28M  ± 4.31K     3.27M  … 3.29M           0 ( 0%)        0%
Benchmark 2 (56 runs): ./target/release/examples/compress 1 rs silesia-small.tar
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          90.5ms ±  541us    88.6ms … 92.0ms          6 (11%)          -  1.2% ±  0.5%
  peak_rss           26.7MB ± 64.8KB    26.5MB … 26.7MB          0 ( 0%)          +  0.0% ±  0.1%
  cpu_cycles          339M  ±  578K      338M  …  341M           0 ( 0%)          -  0.6% ±  0.1%
  instructions        771M  ±  257       771M  …  771M           0 ( 0%)        💩+  3.1% ±  0.0%
  cache_references    398K  ± 2.55K      396K  …  412K           7 (13%)          -  0.1% ±  0.2%
  cache_misses        293K  ± 9.10K      266K  …  304K           9 (16%)          -  0.5% ±  1.2%
  branch_misses      3.20M  ± 10.6K     3.18M  … 3.22M           0 ( 0%)        ⚡-  2.4% ±  0.1%

I suspect the difference in branch mispredictions compared to your results might be due to running on different CPU models. But I haven't yet figured out why my patch causes an increase in instructions. I'll look at it more later.

[brian-pane]

I tried a different approach in 76b6c14: keeping the original logic, but inlining the fast path. It seems to work on my test system.

[folkertdev]

you can try if adding #[cold] here makes a difference. It doesn't really for me. Otherwise this looks good!

[brian-pane]

I just tried with #[cold] and the results look basically unchanged on my system.