Skip to content
New issue

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

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

Already on GitHub? # to your account

std.sync.atomic: extended atomic helper functions #8866

Merged
merged 13 commits into from
May 31, 2021

Conversation

kprotty
Copy link
Member

@kprotty kprotty commented May 22, 2021

  • generic Atomic(T) wrapper replacing atomic.Int and atomic.Bool
  • Bitwise atomic intrinsics when Atomic(T) is an Integer
  • No redundant specification of type as seen in builtins atomic functions
  • fetchXX() ops instead of generic atomicRmw for C11 consistency
  • contains compilerFence() and spinLoopHint()

@kprotty kprotty added the standard library This issue involves writing Zig code for the standard library. label May 22, 2021
@LemonBoy
Copy link
Contributor

We already have atomic wrappers for ints and bools. That approach is much safer than taking raw pointers as it prevents you from mixing atomic and non-atomic ops.

@kprotty
Copy link
Member Author

kprotty commented May 22, 2021

@LemonBoy atomic int and bool dont contain cmpxchg abstractions + there aren't any such wrappers for atomic pointers/enums/floats. Not sure about improved safety as ordering misuse/ignorance can often be equally unsafe. Mixing non-atomic ops and atomic ops can sometimes be valid. Would a generic Atomic(T) type with load/storeUnchecked() and limiting fetchXX()/bitXX() for when T is an Int be a good compromise?

@LemonBoy
Copy link
Contributor

atomic int and bool dont contain cmpxchg abstractions

Should be added if needed.

there aren't any such wrappers for atomic pointers/enums/floats

Wrappers for pointers and enums are also a good idea. Floating point types can be added later (if ever) when somebody finds an use for those.

Not sure about improved safety as ordering misuse/ignorance can often be equally unsafe.

The wrappers try to protect the user from shooting themselves in the foot by checking the ordering parameter, avoid defining meaningless operations, and preventing the mix-up of atomic & non atomic ops. The idea of wrapper types is not novel (see Rust or C11) and strikes a good balance between ergonomics and safety.

Would a generic Atomic(T)

I think separate wrapper types are simpler to define, you need quite a bit of meta programming and usingnamespace to achieve the same effect and define some helpers only when they make sense.

@kprotty
Copy link
Member Author

kprotty commented May 22, 2021

Separate wrapper types feels like repetition: They would all expose load/store/xchg/cmpxchg and ordering checks. Floats would expose that plus arithmetic rmw. Ints would expose even that plus bitwise rmw. Theres few metaprogramming conditions but it covers a lot of cases. Thats my pitch for Atomic(T)

@LemonBoy
Copy link
Contributor

Thats my pitch for Atomic(T)

I'm sold!
Given we're in for some sweeping changes, what do you think of using the same nomenclature as the C++ atomic types (load, store, exchange, fetch{Add,Sub,...}, compareExchange{Strong,Weak},...) ?

@kprotty
Copy link
Member Author

kprotty commented May 23, 2021

I prefer everything except the exchange bits - replacing them for swap/[try]compareAndSwap is on average shorter and is reminiscent of Go's atomic naming scheme.

Actually, another issue with wrapper types is being able to store them inside extern struct/union. Could store the generic bytes and cast between them like atomic_bytes: [@sizeOf(Atomic(T))] align(@alignOf(Atomic(T)) but that is pretty hacky.

@LemonBoy
Copy link
Contributor

Actually, another issue with wrapper types is being able to store them inside extern struct/union

There should be no problem if the wrapper is extern.

@kprotty
Copy link
Member Author

kprotty commented May 26, 2021

freebsd CI: atomic.zig:25:13: error: expected 0 parameters, found 1

atomic.zig:25: @fence(ordering)

Whats up with the fence builtin?

@LemonBoy
Copy link
Contributor

Change this line.

@kprotty
Copy link
Member Author

kprotty commented May 31, 2021

@LemonBoy Any further comments before it gets merged?

Copy link
Contributor

@LemonBoy LemonBoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
The size of type T is actually bounded by the combination of architecture and feature flags being used, an extra check can be added at a later stage though.

@compileError(@tagName(Ordering.Unordered) ++ " is only allowed on atomic loads and stores");
}

comptime var success_is_stronger = switch (failure) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
comptime var success_is_stronger = switch (failure) {
comptime const success_is_stronger = switch (failure) {

@kprotty kprotty merged commit eb6975f into ziglang:master May 31, 2021
Vexu pushed a commit to Vexu/routez that referenced this pull request Oct 17, 2021
Since std.atomic.Int was replaced by std.atomic.Atomic at
"std.sync.atomic: extended atomic helper functions by kprotty · Pull Request #8866 · ziglang/zig"
ziglang/zig#8866
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants