-
Notifications
You must be signed in to change notification settings - Fork 28
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
Use std atomics #276
Use std atomics #276
Conversation
@@ -339,8 +332,8 @@ void DumpResultsToFile() { | |||
struct tm* Tm = gmtime(&Time); | |||
strftime(UtcDateTime, sizeof(UtcDateTime), "%Y.%m.%d-%H:%M:%S", Tm); | |||
fprintf(File, "%s,%u,%u,%u,%u,%u,%u,%u,%u\n", UtcDateTime, | |||
Results.TotalCount, Results.ReachableCount, Results.TooMuchCount, Results.MultiRttCount, Results.RetryCount, | |||
Results.IPv6Count, Results.Quicv2Count, Results.WayTooMuchCount); | |||
Results.TotalCount.load(), Results.ReachableCount.load(), Results.TooMuchCount.load(), Results.MultiRttCount.load(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO always specify the intended memory_order
with atomics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the default not acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memory_order_seq_cst - A load operation with this memory order performs an acquire operation, a store performs a release operation, and read-modify-write performs both an acquire operation and a release operation, plus a single total order exists in which all threads observe all modifications in the same order (see Sequentially-consistent ordering below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all the default std::memory_order_seq_cst
is probably not what makes sense here (the default value is the most conservative and lowest performance option) and second of all, it requires the developer look up the default behavior of a C++ routine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want memory_order_relaxed
which is the opposite end of the spectrum. The same is true for the overloaded ++
operator: using fetch_add(1)
with memory_order_relaxed
is more efficient and clearer to developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called once for the lifetime of the app. Low perf is fine.
Use the standard library for atomic increments.