9

I am checking Clippy findings in my code and found that the pedantic rule needless_pass_by_value might be a false positive.

It says that:

warning: this argument is passed by value, but not consumed in the function body

help: consider taking a reference instead: &Arc<Mutex<MyStruct>>

Since cloning the Arc is only reference counting, moving the Arc should not be bad idea. Does it really make any difference in terms of quality and performance to send a reference instead of a value for the Arc ?

#![warn(clippy::pedantic)]

use std::sync::{Arc, Mutex};

fn main() {
    let my_struct = MyStruct { value: 3 };
    let arc = Arc::new(Mutex::new(my_struct));

    arc_taker(arc.clone());
}

fn arc_taker(prm: Arc<Mutex<MyStruct>>) {
    prm.lock().unwrap().do_something();
}

struct MyStruct {
    value: i32,
}

impl MyStruct {
    fn do_something(&self) {
        println!("self.value: {}", self.value);
    }
}

Playground

Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
Akiner Alkan
  • 6,145
  • 3
  • 32
  • 68

1 Answers1

8

Calling arc_taker(arc.clone()) increments the reference count, and returning from arc_taker decrements it again. This is useless in this case, since the arc variable of main already keeps the Arc alive during the entire call. A reference to it would already suffice. No need to bump the reference count up and down.

In your specific example, arc_taker doesn't even care that it is managed by an Arc. All it cares about is that there is a Mutex to lock, so to make your function less restrictive, just take a &Mutex<MyStruct> instead.

If you wanted to do any Arc-specific things to it, like getting the weak_count or something, taking a &Arc<..> would make sense. If your function would keep a clone of the Arc around, only then it would make sense to take an Arc by value, because then the caller can decide to give you an additional reference to it by calling .clone() (thus bumping the reference count), or to give you ownership of its own Arc (and thus not bumping the reference count).

Mara Bos
  • 955
  • 5
  • 11
  • 3
    Great answer. FWIW, [here's my version of the `&Mutex` solution.](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d21aecae3dbfd7827c8c11f52a13d91a) (Of course, this makes `arc_taker` a bad name since it doesn't take an `Arc`.) – trent Apr 08 '19 at 15:22
  • 5
    It may be worth nothing that there is a general underlying principle here: a function should make only as many assumptions as are required for it to work properly, which should be reflected in the type of arguments it takes. This is the same principle which leads to recommending taking `&str` vs `&String`. – Matthieu M. Apr 08 '19 at 15:37
  • @MatthieuM. Yes, that's a good addition. In this specific case that also means it shouldn't even take a `&Mutex`, since the function always locks it anyway. It should just take a `&MyStruct`, and leave the locking up to the caller. Then callers that already locked the mutex for something else can also call the function, without having to unlock the mutex first. (But I assume the original code where this question came from does more than just `.lock().something()`.) – Mara Bos Apr 08 '19 at 15:40
  • @MatthieuM. I understand the main principle and why I should send the reference by looking the principal. What I am really wondering, breaking that principle does not seem harmful to me apart from it's cost to bump the reference count up and down? – Akiner Alkan Apr 08 '19 at 15:42
  • @M-ou-se my MCVE is not reflecting the need of Arc very well. Let's consider that we are checking the weak_count as well. That does not make much difference untill Arc is consumed by the call. [Playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c6922698906c0fca3754bf277ac25932) – Akiner Alkan Apr 08 '19 at 15:42
  • 2
    @AkinerAlkan I guess I don't know why you would consider this a *false positive* for the clippy lint. The body of `arc_taker` doesn't need to consume the `Arc`, so it can be rewritten to accept a `&Arc`. That's all the lint is saying. You may choose to ignore it for any reason, but that doesn't make it *false*. – trent Apr 08 '19 at 16:00
  • 4
    @AkinerAlkan Breaking that principle means restricting the ways your function can be used. If some other part of your code has an `Rc` or just a `Box`, it's a shame if it can't use your function, because it needlessly requires an `Arc`. If you do need an `Arc`, taking it by value when a reference would suffice means not only a performance hit (writing to an atomic twice), but also makes the interface less ergonomic: `arc_taker(a)` would consume a, so users of the function will need to call `.clone()` instead of just writing `&a`. – Mara Bos Apr 08 '19 at 16:04