-
Notifications
You must be signed in to change notification settings - Fork 46
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
Added a Clear method that resets the read and write indexes. #21
Open
perencia-wc
wants to merge
2
commits into
DNedic:main
Choose a base branch
from
perencia-wc:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
I would also add a test for writing to the buffer after clearing.
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.
I really don't like the first option, and regarding the second I'm not sure to understand why do we need two methods. I'm thinking in something like
wouldn't that be thread safe?
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 operation cannot be thread safe, as it changes both the read and write index. A SPSC queue based data structure works with the assumption that the write index will only ever be changed by the producer and the read index only ever changed by the consumer.
A store with the release memory ordering pairs with a load of the same variable with acquire memory ordering in another thread and they ensure that everything that happens before the release store is visible before the acquire load.
With this in mind, let's take a look at a scenario where the consumer is clearing the buffer, while the producer is writing for the Queue:
The load and store on
_r
ensure that this part will be visible (happens before) to the writer trying to clear the buffer:As this is the only guarantee that we get, it can happen that the writer will think that
_w
is a completely different value from 0 we are setting in the consumer and later down the line we will replace it withw_next
.Even if we were to replace
const size_t w = _w.load(std::memory_order_relaxed);
withconst size_t w = _w.load(std::memory_order_acquire);
, this is not going to work as within the producer and the consumer the ordering of modifications to the indexes is going to be the same, but it would need to be different (and I'm not sure it would work even then, would need to check).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.
As for the second approach, it would be thread safe because the consumer could change the read index to match the write index, whatever it is, and vice versa for the producer.
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.
Thanks for your detailed explanation.
I think I understand the problem, though not the solution 😅.
Thinking it twice, a Clear method is not really necessary, it could be replaced with a kind of "Flush" skipping any remaining items until _r == _w, though this can require to walk all the buffer. But anyway I think some kind of clearing can be convenient, i.e as is our case, when the holder of the buffer is neither the consumer nor the producer, and some event requires the buffer to be cleared/flushed. As I can't provide a reliable solution (at least not until I fully understand it), I think it is better to let you decide if that feature of clearing/flush actually fits in the project and how to implement it 😄
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.
After analyzing this a bit I've found that even
_r = _w
, or doingRead()
in a loop might not be enough, because we can't know if there is a write in progress and,_w
will change immedeately after.The reason I think this is a problem is because usually you want to flush when some assumption has changed and buffered data might not be valid anymore.