RSDK-13439 remove std::memory_order usages#28
Merged
oliviamiller merged 1 commit intoviam-modules:mainfrom Mar 4, 2026
Merged
Conversation
dgottlieb
approved these changes
Mar 3, 2026
seanavery
reviewed
Mar 3, 2026
hexbabe
reviewed
Mar 3, 2026
hexbabe
left a comment
There was a problem hiding this comment.
[optional] performance profiling to get an idea of how performant this opt is
Collaborator
Author
as long as the samples are read before the next portaudio callback theres no performance difference, the overhead from seq_cst is negligible. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The explicit memory_order_relaxed and memory_order_release/acquire usages added unnecessary complexity without benefit.
memory_order_relaxedleaves the CPU and compiler free to reorder the operations, making correctness dependent on the non-obvious happens-before chain through the release on total_samples_written counter. The defaultmemory_order_seq_cstalready performs acquire on loads and release on stores, making the explicit arguments redundant.Switching to using the default memory ordering everywhere to make the code easier to reason about.
ty @dgottlieb for pointing this out to me
1 approval needed.