Yet Another Performance Footgun in Rust - Multithreading Lock Edition

No. "Where the variable name is typed latest" requires you to process way more of the text than "where the block ends".

No. It is often incorrect to drop a lock after a single operation.

1 Like

No, I never see a word "knowing who is the last user of my &mut borrow is hard", eventhough &mut borrow is not as explisit as ASAP drop, because &mut is a type so it is only typed at the definition not the caller, but variable name is a raw text so it will be typed at the caller. This, honestly, sounds like exaggerating

And additionally, this does not need that your manual effort for 95% cases, that thing is only needed most in scope drop to not cause accidental extended lock and deadlock. But ASAP drop is optimised for those footguns by nature, so in the end result, ASAP drop simply doesn't need that manual effort

No, because you didn't even understand what I wrote correctly. Let me point it out, because clearly I wrote this :

Meaning, if you have multiple with to the same variable, the lock is unlocked after the last with, not after every with

That's because it's far more important to know exactly where a lock is released, than to know exactly where a borrow ends. Borrows ending in unexpected places mainly cause annoying compile errors. Locks getting released in unexpected places -- especially locks getting released too early -- cause data races, and data races cause intermittent, hard-to-reproduce runtime failures. I have, more than once, spent multiple weeks pinning down that kind of failure, and once a big chunk of why it took so long was that the sheer volume of visual clutter required for error handling in C confused me into thinking that a lock was released later than it actually was.

But "unlocked after every with" is what the code will look like it does, and that's the whole issue here. When it comes to locking, "the code does what it looks like it does" is more important than any other consideration.

4 Likes

The importanity has nothing to do with the "knowing where ASAP lock is unlocked is easier than knowing where &mut end". It is impossible to cause data race in ASAP lock if the lock guard its inner data, which is what 95% cases are. Eg :

let data = data.lock()

data.update(10)

data.update(20)

Good luck making that code data race, because the lock is automatically extended until the latest place where the word data is typed

For the 5% cases if you feel using it is harder (eventhough actually not, because both can be extended manually, explisit scope == explisit name), you can still use the scope lock, as if you read the discussion from the start, the scope lock is not replaced, ASAP lock is dedicated new type

No, if you write :

let a = with data.update(20)
let b = with data.update(30)

It doesn't imply the lock is unlocked after the with, and there is documentation. If you insist to make the with takes a code block, then it is useless because it just resummons the footguns. I said with is ok if you think it is ok, as long as it still doesn't violate the core design of ASAP lock, as long as it doesn't remove the benefits that ASAP lock want to achieve

I see, if Rust has powerfull compile time capability, this ASAP drop and lock can be done at user space 100% while maintaining its magic, no manual interfere. Compile time capability should be priority because it is a high ceiling feature, it makes custom user space compile time checkers to do things that borrow checker can't do become possible, aka the only limit is user creativity

I care about the 5%. I care about the 5% more than I care about the 95%. For the easy cases, any solution will do; for the difficult cases, it is vital to have a solution that is both correct, and appears to be correct to someone reading in a hurry while tired.

I don't want your ASAP locks to exist, because if they exist at all, someone will try to use them for the difficult cases, and the code will look like it's correct when it isn't.

You can't use documentation to redefine what the code looks like it does. People's intuitions, trained on however many years of experience, including experience with other languages, will tell them that this code drops the lock in between the two statements, and when they read the documentation saying it doesn't, they'll file bug reports because they will think the documentation is wrong.

2 Likes

Then you must remove raw pointer becausw raw pointer is in that 5%. Again, seeing a name is not harder than seeing curly bracket, moreover it is still in the same block (the block where the variable sits)

Again, in that 5% cases, seeing name in the same scope is not harder than seeing curly bracket. It is like saying, seeing manual drop call in the same scope is harder than seeing curly bracket, both requires manual awareness. Your saying only happen if the lock is used for token gating, which is kinda a hack not how to actually use a lock because it does not guard the inner data, which is just 5% cases, compared to the benefit for 95% cases, not making sense. Clippy also has lint to deny some code

The with keyword does not carry : so no component that tells it is a scope. You get plenty of that already, unwrap, expect. But with is not even same to those two, because it tells like "using", using data.update(input). People don't use feature if they don't read the documentation of that feature, because they need to gather info how to use it first. And that is your own idea, not mine, I said if you want to use it do it as it does not violates the goal of ASAP lock, as I said earlier that seeing a continous name in the same scope is not even hard, but you keep exaggerating. ASAP drop and ASAP lock doesn't require keyword with

No, actually it is harder. Much harder. Your brain normally runs an error-correction pass on every word you read before that word reaches your conscious attention. This is why it's so easy to miss typos, but the correction doesn't just hide typos from you, it can replace entire phrases with what "ought" to be there. It takes continuous concentration to suppress this effect.

In the case of the hypothetical feature we're arguing about, this effect is going to make people think the last use of a guarded variable is where it's supposed to be and not where it actually is.

(Curly brace placement is of course also subject to the error filter, but curly braces go with large changes in indentation, and that's usually a big enough visual flag to get past the filter. In fact that's why Rust requires all blocks to be braced -- the experience with C was that people would think indented statements were braced when they weren't.)


I can see that I'm not going to persuade you, and you're not going to persuade me either, so I will stop responding now, but I could not let the above error go unchallenged.

1 Like

+1. In fact, I am not even sure what sane semantics could look like even if all of this was false. What happens to statements in between with blocks? If they are expressions, what happens to other parts of the same expression, especially if with blocks preceded or follow? What kind of reorder magic can or is guaranteed to be done? Are operations outside with blocks guaranteed to be executed with the lock not held?

(These are rhetorical questions. No matter the technical choices, which very well can be made work, I claim that it’s impossible to make this intuitive. This is a footgun.)

On the footgun note, because it does matter - not for terminology, but for assessing the severity of the issue -, this is obviously not a footgun and this is clearly the consensus in this thread. A footgun must be surprising/unexpected or demonstrably hard to avoid (maybe other conditions too). Everything in programming can backfire badly when gotten wrong. It’s all about how likely a mistake is to be made, what its severity is, and how hard it is to identify and resolve.

I think ASAP mutexes can make sense so long as they explicitly make guarantees only for the data they guard with no piggybacking allowed at all. These (per-statement) with blocks, however, sound objectively terrible. Clearer lock scopes, also mentioned in my ramblings above, would be much appreciated.

2 Likes

No, I will give your real visualisation

The 95% cases :

Scope lock :

use std::sync::{Arc, Mutex};
use std::thread;
use std::time::Duration;

struct EngineState {
    metrics: Vec<i32>,
    job_count: usize,
    logs: Vec<String>,
}

fn abc() {
    let master_state = Arc::new(Mutex::new(EngineState {
        metrics: vec![10, 20, 30],
        job_count: 0,
        logs: Vec::new(),
    }));

    let mut workers = vec![];

    for worker_id in 0..3 {
        let state_shared = Arc::clone(&master_state);

        let handle = thread::spawn(move || {
            {
                let mut locked_state = state_shared.lock().unwrap();
                locked_state.job_count += 1;
                locked_state.logs.push(format!("Worker {}", worker_id));        
                
                locked_state.metrics.push(worker_id as i32 * 5);
                if worker_id % 2 == 0 {
                    locked_state.metrics.retain(|&x| x % 2 == 0);
                   
                }           
                
                let inner_len = locked_state.metrics.len();
                locked_state.logs.push(format!("Worker {} {} metric", worker_id, inner_len));           
            }

            {
                let mut locked_metric = state_shared.lock().unwrap();                      
                if let Some(val) = locked_metric.metrics.get_mut(worker_id) {
                    *val *= 2;
                }             
                locked_metric.logs.push(format!("Worker {}", worker_id));
            }
        });
        workers.push(handle);
    }

    for worker in workers {
        worker.join().unwrap();
    }

    {
        let final_state = main_state.lock().unwrap();
        // many more code
    }
}

ASAP lock :

use std::sync::{Arc, ASAP_Mutex};
use std::thread;
use std::time::Duration;

struct EngineState {
    metrics: Vec<i32>,
    job_count: usize,
    logs: Vec<String>,
}

fn abc() {
    let master_state = Arc::new(ASAP_Mutex::new(EngineState {
        metrics: vec![10, 20, 30],
        job_count: 0,
        logs: Vec::new(),
    }));

    let mut workers = vec![];

    for worker_id in 0..3 {
        let state_shared = Arc::clone(&master_state);

        let handle = thread::spawn(move || {
            let mut locked_state = state_shared.lock().unwrap();
            locked_state.job_count += 1;
            locked_state.logs.push(format!("Worker {}", worker_id));        
            locked_state.metrics.push(worker_id as i32 * 5);

            if worker_id % 2 == 0 {
                locked_state.metrics.retain(|&x| x % 2 == 0);
            }
            
            let current_len = locked_state.metrics.len();
            locked_state.logs.push(format!("Worker {} {} metric", worker_id, current_len));

            let mut locked_metric = state_shared.lock().unwrap();
            if let Some(val) = locked_metric.metrics.get_mut(worker_id) {
                *val *= 2;
            }
            locked_metric.logs.push(format!("Worker {}", worker_id));
        });

        workers.push(handle);
    }

    for worker in workers {
        worker.join().unwrap();
    }

    let final_state = main_state.lock().unwrap();
    // many other code
}

Now the 5% case : Lock as token gating

Scope lock :

use std::sync::{Arc, Mutex};
use std::thread;
use std::time::Duration;

fn abc() {
    let gate_a = Arc::new(Mutex::new(()));
    let gate_b = Arc::new(Mutex::new(()));
    let mut workers = vec![];

    for worker_id in 0..3 {
        let lock_a = Arc::clone(&gate_a);
        let lock_b = Arc::clone(&gate_b);

        let handle = thread::spawn(move || {
            {
                let _token_a = lock_a.lock().unwrap();
                println!("Worker {}", worker_id);
                thread::sleep(Duration::from_millis(10));
                println!("Worker {}", worker_id);
            }

            thread::sleep(Duration::from_millis(5));

            {
                let _token_b = lock_b.lock().unwrap();
                println!("Worker {}", worker_id);
                
                if worker_id % 2 == 0 {
                    {
                        let _reentry_a = lock_a.lock().unwrap();
                        println!("Worker {}", worker_id);
                    }
                }
                
                thread::sleep(Duration::from_millis(10));
                println!("Worker {}", worker_id);
            }
        });
        workers.push(handle);
    }

    for worker in workers {
        worker.join().unwrap();
    }

    {
        let _final_gate = gate_a.lock().unwrap();
        println!("Many code");
    }
}

ASAP lock :

use std::sync::{Arc, ASAP_Mutex};
use std::thread;
use std::time::Duration;

fn complex_pipeline_asap() {
    let gate_a = Arc::new(ASAP_Mutex::new(()));
    let gate_b = Arc::new(ASAP_Mutex::new(()));
    let mut workers = vec![];

    for worker_id in 0..3 {
        let lock_a = Arc::clone(&gate_a);
        let lock_b = Arc::clone(&gate_b);

        let handle = thread::spawn(move || {
            let token_a = lock_a.lock().unwrap();
            println!("Worker {}", worker_id);
            thread::sleep(Duration::from_millis(10));
            println!("Worker {}", worker_id);
            drop(token_a);

            thread::sleep(Duration::from_millis(5));

            let token_b = lock_b.lock().unwrap();
            println!("Worker {}", worker_id);
            
            if worker_id % 2 == 0 {
                let reentry_a = lock_a.lock().unwrap();
                println!("Worker {}", worker_id);
                drop(reentry_a);
            }
            
            thread::sleep(Duration::from_millis(10));
            println!("Worker {}", worker_id);
            drop(token_b);
        });
        workers.push(handle);
    }

    for worker in workers {
        worker.join().unwrap();
    }

    let final_gate = gate_a.lock().unwrap();
    println!("Many code");
    drop(final_gate);
}

As you can see, in the 95% cases you don't to care about that manual effort, just the code as you wish, the ASAP lock does the rest to make the lock is correct and optimized out of the box

And in the 5% cases is where ASAP drop has no benefit, as it is no longer magic, but manual annotation, so for this 5% cases you can use scope drop, and no people use ASAP drop if writing this manual annotation in this 5% cases, so you won't need to worry about other people code

So to summary it, scope drop makes you write manual annotation for all cases to get optimal behaviour. But ASAP drop removes the need of doing that manual annotation in the 95% cases, while the 5% cases you can uses scope drop

No, it is clear like this "the lock is unlocked after the line where the variable name is last mentioned, within the scope of where the variable is declared"

There is no with block, with block is his idea, that I corrected if you wanted a uniform word at the start just to make it standout, you can use name with or whatever best name, but "it must only receive 1 line code, no code block be it : or { }. Lock is unlocked after the last keyword that points to the same variable within the scope where said variable is declared". Thus it doesn't change ASAP drop at all, just adding keyword, nothing more

Statement between the keyword has no problem because the lock protects its owned data (this is the 95% cases that ASAP lock try to improve), not the outside operation (the 5% cases, lock as token gate). All other code that sits between the .lock() call to T and the latest keyword that points to said T, operates with knowledge that T is locked, no different with scope block. It is easy to simulate and visualize it, just write code with manual drop call to understand ASAP lock, the difference of ASAP lock is just you don't write that manual drop call but the compiler is the one that does it internally. It is intuitive because it is the same mental model of borrowing

Folks.... This discussion is getting quite heated. I read some of it and I don't think the discussion is fruitful. I'll keep it closed for a bit while I figure out what's going on and chat with ppl in DMs

8 Likes