jzarnett / ece459

ECE 459: Programming for Performance
446 stars 137 forks source link

L16 max in live-coding uses sketchy idiom #62

Closed patricklam closed 3 years ago

patricklam commented 3 years ago

lectures/live-coding/L16/rayon-max-array/src/main.rs uses:

vec.par_iter().for_each(|n| {
    let mut previous_value = max.load(Ordering::SeqCst);
    if *n > previous_value {
        while max.compare_and_swap(previous_value, *n, Ordering::SeqCst) != previous_value {
            println!("Compare and swap was unsuccessful; retrying");
            previous_value = max.load(Ordering::SeqCst);
        }
    }
});

which may have a race condition between previous_value and the new max; the code in L16.tex uses a loop (as recommended) instead:

    loop {
        let old = max.load(Ordering::SeqCst);
        if *n <= old {
            break;
        }
        let returned = max.compare_and_swap(old, *n, Ordering::SeqCst);
        if returned == old {
            println!("Swapped {} for {}.", n, old);
            break;
        }
    }

Both code snippets were introduced in the same commit on October 14. Assigning to @jzarnett who wrote this code.

jzarnett commented 3 years ago

Ah. This is one of those situations where the code that I committed in this file is the original code containing an error. Then the video goes over it, explains why it's wrong, and produces the final version in the tex file. I think either the final version should appear in the code, or else a warning should appear in the source file that says this is buggy and should not be used. Leaving it wrong without a warning isn't cool. Feel free to replace the initial with the final version or add the warning, at your option. If you don't get to it quickly enough then I'll do one or the other.

patricklam commented 3 years ago

Thanks! I've put warnings in the demo code. There is an issue with demonstrating things that are wrong in some situations: people imprint on it and under pressure the wrong code comes out. I think that in this particular case the risks are low for multiple reasons, so I'm more fine with the code containing an error.

There's interesting pedagogical thinking to be done here. In a large class setting asking for the audience to find the error is not the right thing (most won't engage and it's a chance to show off). It might be appropriate for an in-class exercise in pairs where students have to find the problem and then we show the fix.

patricklam commented 3 years ago

probably next time we look at the videos we should put some sort of warning on the initial version in the slides as well.