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

Support for atomics data types? #1496

Closed
siscia opened this issue Mar 15, 2019 · 5 comments · Fixed by #1572
Closed

Support for atomics data types? #1496

siscia opened this issue Mar 15, 2019 · 5 comments · Fixed by #1572

Comments

@siscia
Copy link

siscia commented Mar 15, 2019

Hi,

it seems like there is no Serialize support for atomic data type: https://doc.rust-lang.org/core/sync/atomic/index.html

Is there any reason for this?

A first implementation would be to simply load them using Ordering::Relaxed and that should be quite enough.

@dtolnay
Copy link
Member

dtolnay commented Mar 15, 2019

I would prefer to match Debug which uses Ordering::SeqCst.
https://github.com/rust-lang/rust/blob/1.33.0/src/libcore/sync/atomic.rs#L1137-L1141

I would be prepared to consider a PR adding impls that use SeqCst.

@siscia
Copy link
Author

siscia commented Mar 15, 2019

SeqCst is the strongest guarantee possible. This is ok and maybe even desiderable in debugging code, but I am not sure is desiderable on production code.

What is your take on this?

Do you believe performance won't be a concern? I am not sure of the performance characteristics of serde (never used) so I am really asking a little more feedback on your thoughts!

Cheers

@dtolnay
Copy link
Member

dtolnay commented Mar 17, 2019

If someone observes serialization of atomics as a bottleneck, they can use a serialize_with attribute to substitute in a Serialize impl with a weaker guarantee. SeqCst is a safe default with the least surprises and frequently no different performance.

@siscia
Copy link
Author

siscia commented Mar 17, 2019

I am not sure if I got the time to do this anyway, I don't believe in the next few weeks at the very least.

If anybody wants to jump on it feel free to just implement it!

@Roguelazer
Copy link
Contributor

I believe #1572 solves this (and is ready for review).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants