[ch04/global-vars/src/main.rs] -- Redundant Use of Arc<Mutex<>> in thread_local!

[DannyCeb]

I've identified an inefficient pattern in the code where Arc<Mutex<Option<Vec>>> is used inside a thread_local! macro. This combination is redundant and represents an unnecessary use of atomic operations.

thread_local! {
    static POPULAR_BABY_NAMES_2021: Arc<Mutex<Option<Vec<String>>>> =
        Arc::new(Mutex::new(None));
}

I recommend changing to Rc<RefCell<Vec>> like the following example

use std::cell::RefCell;
use std::rc::Rc;

thread_local! {
    static POPULAR_BABY_NAMES_2021: Rc<RefCell<Vec<String>>> =
        Rc::new(RefCell::new(vec![]));
}

fn main() {
    let rc: Rc<RefCell<Vec<String>>> = POPULAR_BABY_NAMES_2021.with(|rc: &Rc<RefCell<Vec<String>>>| rc.clone());
    let mut inner: std::cell::RefMut<'_, Vec<String>> = rc.borrow_mut();
    inner.extend(vec![
        String::from("Olivia"),
        String::from("Liam"),
        String::from("Emma"),
        String::from("Noah"),
    ]);

    println!("popular baby names of 2021: {:?}", inner);
}

[brndnmtthws]

Thanks for filing this. You're right: inside thread_local! the value is per-thread and never shared across threads, so the Arc + Mutex combo is redundant and just adds atomic overhead and locking. A RefCell (or std::cell::OnceCell if we still want one-time init) is enough.

For this example I'd switch to something like:

use std::cell::RefCell;
use std::rc::Rc;

thread_local! {
    static POPULAR_BABY_NAMES_2021: Rc<RefCell<Vec<String>>> = Rc::new(RefCell::new(Vec::new()));
}

fn main() {
    POPULAR_BABY_NAMES_2021.with(|rc| {
        rc.borrow_mut().extend([
            "Olivia".to_string(),
            "Liam".to_string(),
            "Emma".to_string(),
            "Noah".to_string(),
        ]);
    });

    let names = POPULAR_BABY_NAMES_2021.with(|rc| rc.borrow().clone());
    println!("popular baby names of 2021: {:?}", names);
}

If we want lazy one-time initialization per thread, thread_local! with an unsync::OnceCell would also work. Happy to patch this if you agree.

[brndnmtthws]

Great catch again. Would you be up for opening a small PR to swap the thread-local to Rc<RefCell<Vec<String>>> (or unsync::OnceCell if you prefer one-time init)? I'm happy to review/merge.

[DannyCeb]

Thanks for your reply, I'll open a PR

[DannyCeb]

I have the changes ready in my local repository.
I tried to keep most of the file untouched in order to maintain consistency with the book version.
Would it be appropriate to add a comment in main.rs to clarify the change, perhaps as an errata note?

The current code looks like:

use lazy_static::lazy_static;
use once_cell::sync::Lazy;
use static_init::dynamic;
use std::cell::OnceCell;
use std::cell::RefCell;
use std::rc::Rc;

thread_local! {
    static POPULAR_BABY_NAMES_2021: Rc<RefCell<Vec<String>>> = Rc::new(RefCell::new(Vec::new()));
}

lazy_static! {
    static ref POPULAR_BABY_NAMES_2020: Vec<String> = {
        vec![
            String::from("Olivia"),
            String::from("Liam"),
            String::from("Emma"),
            String::from("Noah"),
        ]
    };
}

static POPULAR_BABY_NAMES_2019: Lazy<Vec<String>> = Lazy::new(|| {
    vec![
        String::from("Olivia"),
        String::from("Liam"),
        String::from("Emma"),
        String::from("Noah"),
    ]
});

#[dynamic]
static POPULAR_BABY_NAMES_2018: Vec<String> = vec![
    String::from("Emma"),
    String::from("Liam"),
    String::from("Olivia"),
    String::from("Noah"),
];

fn main() {
    POPULAR_BABY_NAMES_2021.with(|rc| {
        rc.borrow_mut().extend([
            "Olivia".to_string(),
            "Liam".to_string(),
            "Emma".to_string(),
            "Noah".to_string(),
        ]);
    });

    let popular_baby_names_2017: OnceCell<Vec<String>> = OnceCell::new();
    popular_baby_names_2017.get_or_init(|| {
        vec![
            String::from("Emma"),
            String::from("Liam"),
            String::from("Olivia"),
            String::from("Noah"),
        ]
    });

    let names = POPULAR_BABY_NAMES_2021.with(|rc| rc.borrow().clone());
    println!("popular baby names of 2021: {:?}", names);

    println!("popular baby names of 2020: {:?}", *POPULAR_BABY_NAMES_2020);

    println!("popular baby names of 2019: {:?}", *POPULAR_BABY_NAMES_2019);

    println!("popular baby names of 2018: {:?}", *POPULAR_BABY_NAMES_2018);

    println!(
        "popular baby names of 2017: {:?}",
        popular_baby_names_2017.get()
    );
}

[DannyCeb]

Here is the PR link: #24