Add max spin count limit to circular buffer #1088
Merged
Conversation
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
reyang
commented
Aug 16, 2020
| @@ -113,7 +113,7 @@ internal long ProcessedCount | |||
| /// <inheritdoc/> | |||
| public override void OnEnd(Activity activity) | |||
| { | |||
| if (this.queue.TryAdd(activity)) | |||
| if (this.queue.TryAdd(activity, maxSpinCount: 50000)) | |||
There was a problem hiding this comment.
Do we want to make this configurable?
reyang
commented
Aug 16, 2020
|
|
||
| var spinCountDown = maxSpinCount; | ||
|
|
||
| while (true) |
There was a problem hiding this comment.
Duplicate the code from Add instead of reusing the code - for better perf.
reyang
commented
Aug 16, 2020
| /// Attempts to add the specified item to the buffer. | ||
| /// </summary> | ||
| /// <param name="value">The value to add.</param> | ||
| /// <param name="maxSpinCount">The maximum allowed spin count, when set to a negative number of zero, will spin indefinitely.</param> |
There was a problem hiding this comment.
We might want to avoid spin entirely if we are running on a single core machine. I guess it is a low priority at this moment.
ThomsonTan
reviewed
Aug 16, 2020
Codecov Report
@@ Coverage Diff @@
## master #1088 +/- ##
==========================================
- Coverage 76.01% 75.85% -0.16%
==========================================
Files 225 225
Lines 6208 6225 +17
==========================================
+ Hits 4719 4722 +3
- Misses 1489 1503 +14
|
CodeBlanch
approved these changes
Aug 17, 2020
CodeBlanch
reviewed
Aug 17, 2020
| } | ||
| } | ||
| } | ||
|
|
||
| public IEnumerable<T> Consume(int count) |
There was a problem hiding this comment.
@reyang FYI just noticed you don't have any XML comments for Consume. Also might want to rename count -> maxCount or something, because you can actually get less?
There was a problem hiding this comment.
Good catch, let me send a small PR to fix it.
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.
@ThomsonTan brought up a good point that we might want to avoid spinning indefinitely.
This could happen on a highly concurrent environment since spin lock does not guarantee ordering, so mathematically it is possible that one unfortunate thread ended up to be always spinning while in its own quorum, which is scary for customers who use telemetry SDK.