Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

could std::intrinsics::atomic_max be exposed via an AtomicUsize::fetch_max method? #48384

Closed
spacejam opened this issue Feb 20, 2018 · 3 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@spacejam
Copy link

spacejam commented Feb 20, 2018

As part of my recent work on supporting transactions in sled I've wanted to take advantage of the atomic_max intrinsic so that I can implement a lock-free concurrency control protocol more efficiently, without spinning on CAS in a loop. It would be wonderful if I could do this without relying on compiling C. Specifically, updating the read timestamp of an item read in an MVOCC transaction, where read items need to be marked as having been read "at least as high as" a transaction timestamp being executed by a particular thread.

Are there any blockers to adding this? I'm happy to write the code that exposes it if not.

@pietroalbini pietroalbini added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Feb 20, 2018
@hanna-kruppe
Copy link
Contributor

Note that if the hardware does not support atomic max, this is expanded to a CAS loop regardless. At least on the default x86_64 target that is the case (see playground).

@spacejam
Copy link
Author

Ahh yeah, even with target-cpu=native on an i7. Dang, I let a wait-free fantasy develop without first verifying how the intrinsic works :P

    6f30:       48 83 f8 06             cmp    $0x6,%rax
    6f34:       b9 07 00 00 00          mov    $0x7,%ecx
    6f39:       48 0f 4f c8             cmovg  %rax,%rcx
    6f3d:       f0 48 0f b1 4c 24 08    lock cmpxchg %rcx,0x8(%rsp)
    6f44:       75 ea                   jne    6f30 <_ZN3amx4main17h5557c2e4c983c622E+0x20>

Anyway, I think introducing a max method to AtomicUsize would make this sort of operation less bug-prone and more accessible, as lots of people get (rightfully) scared away by the idea of writing CAS loops, or in more cases are probably not aware that this sort of thing can be done atomically. Still happy to introduce this if there is openness to it! I know I would use it :)

llogiq added a commit to llogiq/rust that referenced this issue Mar 30, 2018
This adds a new method to all numeric `Atomic*` types with a
safe compare-and-set loop, so users will no longer need to write
their own, except in *very* strange circumstances.

This solves rust-lang#48384 with `x.fetch_max(_)`/`x.fetch_min(_)`. It
also relates to rust-lang#48655 (which I misuse as tracking issue for now).

*note* This *might* need a crater run because the functions could
clash with third party extension traits.
kennytm added a commit to kennytm/rust that referenced this issue Apr 5, 2018
Add a generic CAS loop to std::sync::Atomic*

This adds two new methods to both `AtomicIsize` and `AtomicUsize` with optimized safe compare-and-set loops, so users will no longer need to write their own, except in *very* strange circumstances.

`update_and_fetch` will apply the function and return its result, whereas `fetch_and_update` will apply the function and return the previous value.

This solves rust-lang#48384 with `x.update_and_fetch(|x| x.max(y))`. It also relates to rust-lang#48655 (which I misuse as tracking issue for now)..

*note* This *might* need a crater run because the functions could clash with third party extension traits.
@Mark-Simulacrum
Copy link
Member

This has since been added (#48655).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants