[ptr_arg] writing `&Vec<_>` instead of `&[_]` has a important performance impact (~20%)

[jeremie-H]

Summary

Hello !

I've noticed that Clippy suggest me to change a &Vec<Vec<u8>> into &[Vec<u8>] but performances are worse

Here my function :

fn get_pixel(i: i32, j: i32, image: &Vec<Vec<u8>>, infinite_value: usize) -> usize {
   ...
}

my function get_pixel is called a lot, here a summary :

fn calcul_pixel_sorti( ... image: &Vec<Vec<u8>> ... ) -> u8 {
...  
  for ...
    for ...
      get_pixel(i, j, image, infinite_value);
...
}

The clippy message

warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices
  --> src/day20/src/lib.rs:99:37
   |
99 | fn get_pixel(i: i32, j: i32, image: &Vec<Vec<u8>>, infinite_value: usize) -> usize {
   |                                     ^^^^^^^^^^^^^ help: change this to: `&[Vec<u8>]`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

If i use this function signature :

fn get_pixel(i: i32, j: i32, image: &Vec<Vec<u8>>, infinite_value: usize) -> usize {

, the full computation take : 8,12 ms

but if i follow the clippy recommandation, with this signature :

fn get_pixel(i: i32, j: i32, image: &[Vec<u8>], infinite_value: usize) -> usize {

it take 9,68ms
So i think Clippy shouldn't recommend this kind of checks, because 19% slower is bad from my point of view :(

Lint Name

ptr_arg

Reproducer

I've attached an example to run, (it's the resolution of day 20 in advent-of-code ;-) )

The line to change is in src/day20/src/lib.rs, line 100, informations are described in the Readme.md

clippy_ptr_arg.zip

Version

rustc 1.60.0-nightly (17d29dcdc 2022-01-21)
binary: rustc
commit-hash: 17d29dcdce9b9e838635eb0adefd9b8b1588410b
commit-date: 2022-01-21
host: x86_64-unknown-linux-gnu
release: 1.60.0-nightly
LLVM version: 13.0.0

Additional Labels

No response

[Jarcho]

The benchmark as written has a variance of about 15%-20% from my testing. It seems like it's very sensitive to scheduling differences. After removing the println from the test and increasing the runs to 1000 rather than 50 I get a difference between the two versions of about 2%-3% favouring the Vec version. If #[inline(never)] is added to get_pixel then the results flip in favour of the slice version with closer to a 4% difference. Not optimized code has the slice version faster as well.

This looks like it's an issue with the optimizer on this specific code. I couldn't tell you what it's doing wrong here. Debugging optimized code is not exactly fun either.

This was tested on a haswell/win10 machine.

[jeremie-H]

hum, it's quite weird, my variance is about 1, 2% only (after running dozen of times)
and it's quite obvious that this change add delay on the bench from my side (about 20%)

What is your rust version ?

[jeremie-H]

After removing println and increasing the runs to 1000 like your test, i've got exactly the same performance impact for me

with : &Vec<Vec<u8>>
day20 p1 took 168.52 μs
day20 p2 took 7.99 ms

with : &[Vec<u8>]
day20 p1 took 197.23 μs
day20 p2 took 9.61 ms

I've tested on another device, and i've got same results about ~20%

[matthiaskrgr]

Might be that llvm vectorizes &Vec<Vec<u8>> and &[Vec<u8>] differently.
You can try debugging llvm optimizations with
RUSTFLAGS="-C llvm-args=--pass-remarks-missed=loop-vectorize -Cdebuginfo=2" cargo build --release

[jeremie-H]

I don't know how to deal with the debug informations :) but the command RUSTFLAGS="-C llvm-args=--pass-remarks-missed=loop-vectorize -Cdebuginfo=2" cargo run -p bench --release get the same output, except the last lines which are interleaved :

remark: /rustc/17d29dcdce9b9e838635eb0adefd9b8b1588410b/library/core/src/option.rs:905:9: loop not vectorized
remark: src/main.rs:30:31: loop not vectorized
remark: src/main.rs:18:25: loop not vectorized

remark: /rustc/17d29dcdce9b9e838635eb0adefd9b8b1588410b/library/core/src/option.rs:905:9: loop not vectorized
remark: src/main.rs:18:25: loop not vectorized
remark: src/main.rs:30:31: loop not vectorized

[matthiaskrgr]

Mhh.
I ran RUSTFLAGS="-C llvm-args=--pass-remarks=.* -Cdebuginfo=2" cargo build --release |& grep src/lib.rs now and compared both optimization remarks diff -u vecvec slicevec

-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:17: hoisting icmp
-remark: src/day20/src/lib.rs:102:22: hoisting trunc
-remark: src/day20/src/lib.rs:102:22: hoisting trunc
-remark: src/day20/src/lib.rs:102:22: hoisting trunc
-remark: src/day20/src/lib.rs:102:23: hoisting load
-remark: src/day20/src/lib.rs:102:23: hoisting load
-remark: src/day20/src/lib.rs:102:23: hoisting load
+remark: src/day20/src/lib.rs:102:8: hoisting icmp
+remark: src/day20/src/lib.rs:102:8: hoisting icmp
+remark: src/day20/src/lib.rs:102:8: hoisting icmp
+remark: src/day20/src/lib.rs:102:8: hoisting select
+remark: src/day20/src/lib.rs:102:8: hoisting select
+remark: src/day20/src/lib.rs:102:8: hoisting select
+remark: src/day20/src/lib.rs:102:8: hoisting trunc
 remark: src/day20/src/lib.rs:105:27: sinking zext
 remark: src/day20/src/lib.rs:105:27: sinking zext
 remark: src/day20/src/lib.rs:105:27: sinking zext
@@ -25,6 +17,24 @@
 remark: src/day20/src/lib.rs:105:27: sinking zext
 remark: src/day20/src/lib.rs:105:27: sinking zext
 remark: src/day20/src/lib.rs:105:27: sinking zext
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting getelementptr
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
+remark: src/day20/src/lib.rs:105:9: hoisting icmp
 remark: src/day20/src/lib.rs:141:1: _ZN4core3ptr46drop_in_place$LT$alloc..vec..Vec$LT$u8$GT$$GT$17h22a3679f40adfebeE inlined into _ZN5day2010load_input17h62fc9509553efabcE with (cost=20, threshold=45) at callsite _ZN5day2010load_input17h62fc9509553efabcE:26:1;
 remark: src/day20/src/lib.rs:17:15: _ZN4core4iter6traits8iterator8Iterator3sum17h5109df11928d8becE inlined into _ZN5day205part117h2a974036d9e67896E with (cost=25, threshold=250) at callsite _ZN5day205part117h2a974036d9e67896E:11:15;
 remark: src/day20/src/lib.rs:17:15: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day205part117h2a974036d9e67896E with (cost=-25, threshold=375) at callsite _ZN5day205part117h2a974036d9e67896E:11:15;
@@ -44,3 +54,15 @@
 remark: src/day20/src/lib.rs:59:25: hoisting load
 remark: src/day20/src/lib.rs:60:89: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h062bb40aaef00d4bE inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:89;
 remark: src/day20/src/lib.rs:69:1: _ZN4core3ptr69drop_in_place$LT$alloc..vec..Vec$LT$alloc..vec..Vec$LT$u8$GT$$GT$$GT$17h78a33e418f41df0cE inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=125, threshold=250) at callsite _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:14:1;
+remark: src/day20/src/lib.rs:78:29: hoisting shl
+remark: src/day20/src/lib.rs:78:59: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:4:59 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;
+remark: src/day20/src/lib.rs:79:37: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:5:37 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;
+remark: src/day20/src/lib.rs:80:41: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:6:41 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;
+remark: src/day20/src/lib.rs:82:29: hoisting shl
+remark: src/day20/src/lib.rs:82:55: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:8:55 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;
+remark: src/day20/src/lib.rs:83:33: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:9:33 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;
+remark: src/day20/src/lib.rs:84:37: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:10:37 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;
+remark: src/day20/src/lib.rs:86:32: hoisting shl
+remark: src/day20/src/lib.rs:86:62: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:12:62 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;
+remark: src/day20/src/lib.rs:87:37: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:13:37 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;
+remark: src/day20/src/lib.rs:88:41: _ZN72_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..ops..deref..Deref$GT$5deref17h177673b3729d2482E inlined into _ZN5day2018enhance_image_once17h0f34de93e44bde3fE with (cost=-15, threshold=787) at callsite _ZN5day2018calcul_pixel_sorti17h2037d1741c6ba009E:14:41 @ _ZN5day2018enhance_image_once17h0f34de93e44bde3fE:5:42;

I'm not an llvm expert or anything, but it seems that with the Vec<Vec..>> function, llvm manages to do some additional inlining and hoisting (moving computations from inside a loop outside ouf a loop) that it does not do with the &[Vec<..>] version of the function. 🤔

[Jarcho]

This is the assembly diff on godbolt: https://godbolt.org/z/ob84hjaTT (slice is left, vec is right)

On a quick look this looks more or less the same between the two. Most of the differences look like just minor rearrangements but there are a few spots I'm not sure about. It's a large chunk of assembly and would take a while to go through.

Edit:
The biggest oddity is the beginning of the outer loop (line 815 or 817). The slice version is doing some unnecessary work, which leads to some extra branches later. Other than that parts have some minor rearrangements, but none of it looks like it would lead to a big performance difference. I'll guess that the performance difference @jeremie-H and I are seeing is probably cpu architecture differences magnifying the problem on their end.

[jeremie-H]

Hello !
It seems that someone in the mentioned issue above get the same performance impact, so they allow the clippy ptr_arg.

Shouldn't we deactivate this recommandation since it's a real performance impact ?

[Shnatsel]

Shouldn't we deactivate this recommandation since it's a real performance impact ?

It can also result in significant speedups, e.g. applying it here resulted in a 15% performance improvement.

But people who see performance improvements do not complain about them on the bug tracker, so the results posted here have some sampling bias.

In theory the slice version should optimize better for multiple reasons, see #10269

[jeremie-H]

Shouldn't we deactivate this recommandation since it's a real performance impact ?

It can also result in significant speedups, e.g. applying it here resulted in a 15% performance improvement.

But people who see performance improvements do not complain about them on the bug tracker, so the results posted here have some sampling bias.

In theory the slice version should optimize better for multiple reasons, see #10269

From my point of view, some speedup, times to times (and there's no metrics to know how many), should never hide performance drop issues.

I prefer to get no recommandations by clippy in this state