Simplify AtomicBool::fetch_nand

[mstewartgallus]

This should also be faster.

This code avoids storing in cases where there would not be a change.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Looks like tests are failing?

[nagisa]

I think you can simply self.swap(true, order) if val is false.

[mstewartgallus]

@nagis That is true. However, a swap implies a write all the time instead while a compare and swap does not which can be faster sometimes. In any case the bug was in the if val part so I replaced that with a fetch_xor. The commit will be up in a bit.

[alexcrichton]

The lack of a loop around the false branch here looks very worrisome to me, is there a reason why there's no loop around that?

[mstewartgallus]

@alexcrichton That's deliberate. !(x && false) = true. If x is true and not false then no write needs to happen which is good for the cache.

[eddyb]

I would use & instead of && in these, seems clearer.

[eddyb]

Explicit return isn't needed.

[mstewartgallus]

This is odd https://travis-ci.org/rust-lang/rust/jobs/213176849 I can't view what happened here.

[nagisa]

To me this all seems correct. I still think atomoc_swap would be better in a sense that it's easier to see it's obviously correct unlike he CAS but either way seems correct to me.

…

On Mar 21, 2017 05:54, "Steven Stewart-Gallus" ***@***.***> wrote: This is odd https://travis-ci.org/rust-lang/rust/jobs/213176849 I can't view what happened here. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#40563 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0lTDx9MPxut9YjQeAv9u25dJL9L3ks5rn0n2gaJpZM4Metst> .

[eddyb]

@sstewartgallus Only one configuration is supposed to run on a PR, but the only way to do that in travis is to finish immediately for non-PR configurations. But macOS ones sometimes even fail at that.
I've restarted it for you, should show green in a couple minutes.

[alexcrichton]

Do you have benchmarks for this as well to justify this change?

[mstewartgallus]

@alexcrichton Fair enough. However, as this is in libcore I can't easily use std::threads for benchmarking purposes can I?

[eddyb]

@sstewartgallus You can write your own standalone benchmark.

[ghost]

Benchmark: source
Results:

test new_ce_1t   ... bench:      89,904 ns/iter (+/- 7,130)
test new_ce_2t   ... bench:     316,835 ns/iter (+/- 367,981)
test new_ce_4t   ... bench:   1,005,424 ns/iter (+/- 320,508)
test new_swap_1t ... bench:      90,034 ns/iter (+/- 12,027)
test new_swap_2t ... bench:     315,171 ns/iter (+/- 297,658)
test new_swap_4t ... bench:   1,000,136 ns/iter (+/- 139,235)
test old_1t      ... bench:     146,440 ns/iter (+/- 20,576)
test old_2t      ... bench:     561,456 ns/iter (+/- 715,659)
test old_4t      ... bench:   2,822,821 ns/iter (+/- 1,544,049)

Pretty good imprevements! :)

Let's just get rid of that unnecessary return and add a test for fetch_nand to src/libcore/tests/atomic.rs.

[ghost]

Let's close this stale PR, as we've pushed the changes forward through a newer PR.
I'd like to thank @sstewartgallus - this is an excellent performance improvement! :)