Fix RingBuffer::push() loading wrong atomic index#697
Merged
Samahu merged 1 commit intoouster-lidar:masterfrom Feb 23, 2026
Merged
Fix RingBuffer::push() loading wrong atomic index#697Samahu merged 1 commit intoouster-lidar:masterfrom
Samahu merged 1 commit intoouster-lidar:masterfrom
Conversation
In push(), the initial load reads r_idx_ (read index) instead of w_idx_ (write index) before the compare_exchange_strong loop on w_idx_. This causes the CAS to always fail on the first iteration when the buffer is non-empty, since the expected value comes from the wrong index. The CAS self-corrects on retry (failure loads the actual w_idx_ value), so this is a performance bug rather than a correctness bug, but the intent is clearly to load w_idx_ — matching the pattern in pop() which correctly loads r_idx_.
Samahu
approved these changes
Feb 18, 2026
| throw std::overflow_error("pushed a full ring buffer"); | ||
| } | ||
| size_t write_idx = r_idx_.load(); | ||
| size_t write_idx = w_idx_.load(); |
Collaborator
There was a problem hiding this comment.
Make sense.. makes me wonder how this wasn't causing problem thus far.
matthew-lidar
approved these changes
Feb 23, 2026
Collaborator
|
@EasonNYC We have merged the PR but please note this object might be removed on the next update (since the current code doesn't depend on it). |
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
4 participants
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.
Summary
RingBuffer::push()inouster_client/include/ouster/impl/ring_buffer.hloadsr_idx_(the read index) instead ofw_idx_(the write index) before thecompare_exchange_strongloop. This causes the CAS to always fail on the first iteration when the buffer is non-empty, adding an unnecessary retry on every push.The fix changes line 151 from:
size_t write_idx = r_idx_.load();to:
size_t write_idx = w_idx_.load();This matches the correct pattern used in
pop(), which loadsr_idx_before its CAS loop onr_idx_.Impact
Eliminates one wasted CAS iteration per
push()call on non-empty buffers.