Refactor exporter - step 4 #1085

reyang · 2020-08-15T18:29:26Z

This is a follow up PR of #1078.

Changes

Added a multi-writer single-reader circular buffer, which will be used for batch exporting scenario. Key points:
- The entire buffer is pre-allocated.
- Consumer doesn't need to copy items to a new array/list, the consumption is happening "in-place".
Still need to read more about the CLR/JIT memory model (about the Int64 operation) to see if additional memory barrier should be added.
I'll do some initial perf test offline, additional optimization can be done later without having the affect the interface.
IEnumerator optimization will be covered separately in Perf improvement using zero-alloc foreach #1080.
[Don't merge] Stress test for CircularBuffer #1086 has the stress test code FYI.

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public API reviewed

codecov · 2020-08-15T18:36:13Z

Codecov Report

Merging #1085 into master will decrease coverage by 0.42%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1085      +/-   ##
==========================================
- Coverage   76.59%   76.17%   -0.43%     
==========================================
  Files         224      225       +1     
  Lines        6165     6199      +34     
==========================================
  Hits         4722     4722              
- Misses       1443     1477      +34

Impacted Files	Coverage Δ
src/OpenTelemetry/Internal/CircularBuffer.cs	`0.00% <0.00%> (ø)`

src/OpenTelemetry/Internal/CircularBuffer.cs

CodeBlanch · 2020-08-15T20:53:29Z

src/OpenTelemetry/Internal/CircularBuffer.cs

+
+                if (this.SwapIfNull(index, value))
+                {
+                    this.head = headSnapshot + 1;


I know it's a really really big number, but in theory it could wrap around to negative at some point which would throw overflow exception I think? If we switched to ulong head/tail and did unchecked (this.head = headSnapshot + 1) I think the wrap would be fine given the % this.capacity in the algorithm?

I guess it is impractical to hit that number? Adding the wrapping support requires more than unchecked, it will touch other part of the lock-free algorithm and put additional requirements on memory barriers. So I would prefer to keep it as-is in favor of perf vs. mathematical correctness.

timr-dev · 2020-08-15T22:26:56Z

src/OpenTelemetry/Internal/CircularBuffer.cs

+
+            count = Math.Min(count, this.Count);
+
+            for (int i = 0; i < count; i++)


Do you want to provide an additional check with a boolean and an Interlock to prevent 2 threads from accidently entering if the single reader pattern is accidently broken.

ThomsonTan · 2020-08-15T22:54:45Z

src/OpenTelemetry/Internal/CircularBuffer.cs

+
+                if (this.SwapIfNull(index, value))
+                {
+                    if (Interlocked.CompareExchange(ref this.head, headSnapshot + 1, headSnapshot) == headSnapshot)


Is headSnapshot validation needed, or Interloaded.Add can work here?

It's is required, otherwise it'll break the 🧙‍♀️.

I see, yes, it is required because the item pointed by headSnapshot maybe not in the range of [this.tail, this.head).

CodeBlanch

LGTM

ThomsonTan · 2020-08-16T18:57:48Z

As the name implied it is a circular buffer, should it TryAdd overwrite the oldest item when the buffer is full instead of returning false on the most recent item?

ThomsonTan · 2020-08-16T19:01:01Z

src/OpenTelemetry/Internal/CircularBuffer.cs

+                throw new ArgumentNullException(nameof(value));
+            }
+
+            while (true)


Retry here introduces uncertainty of running time of TryAdd, perhaps return false instead of retry in the while(true) loop, or limit retry count?

The intention of this function is to return false only when queue is full.

@ThomsonTan you've actually worried me! 😅 Here goes a PR #1088 to address it.

ThomsonTan · 2020-08-16T19:39:29Z

src/OpenTelemetry/Internal/CircularBuffer.cs

+        private long head = 0;
+        private long tail = 0;
+
+        public CircularBuffer(int capacity)


Restrict capacity to be power of 2, so the modulo operation % could replaced with a simple bitwise and?

How much (X%) perf improvement would you expect from modulo?

div could take tens times of CPU cycles than bitwise arithmetic instruction, but there is also trade off taking more memory (for ceiling(2**size)). This could be a low priority optimization and needs data to prove.

ThomsonTan · 2020-08-16T19:45:20Z

src/OpenTelemetry/Internal/CircularBuffer.cs

+        {
+            if (count <= 0)
+            {
+                yield break;


When the queue is empty, wait for notification of new items instead of polling?

That would make it a normal lock based queue instead of lock-free.

add a circular buffer

0cb0540

reyang requested a review from a team August 15, 2020 18:29

CodeBlanch reviewed Aug 15, 2020

View reviewed changes

src/OpenTelemetry/Internal/CircularBuffer.cs Outdated Show resolved Hide resolved

CodeBlanch reviewed Aug 15, 2020

View reviewed changes

add memory barrier

5d48df3

reyang mentioned this pull request Aug 15, 2020

[Don't merge] Stress test for CircularBuffer #1086

Closed

++

87f2fd4

timr-dev reviewed Aug 15, 2020

View reviewed changes

timr-dev approved these changes Aug 15, 2020

View reviewed changes

ThomsonTan reviewed Aug 15, 2020

View reviewed changes

clean up

590cf40

CodeBlanch approved these changes Aug 16, 2020

View reviewed changes

reyang added the pr:please-merge label Aug 16, 2020

Merge branch 'master' into reyang/buffer

e196f78

CodeBlanch merged commit 90c370f into master Aug 16, 2020

CodeBlanch deleted the reyang/buffer branch August 16, 2020 05:13

ThomsonTan reviewed Aug 16, 2020

View reviewed changes

reyang mentioned this pull request Aug 16, 2020

Add max spin count limit to circular buffer #1088

Merged

reyang mentioned this pull request Oct 26, 2021

Test thread safety during CI #2513

Closed

reyang mentioned this pull request Dec 15, 2023

Scope info should only be exported for Prometheus if OpenMetrics is requested #5182

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor exporter - step 4 #1085

Refactor exporter - step 4 #1085

reyang commented Aug 15, 2020 •

edited

Loading

codecov bot commented Aug 15, 2020 •

edited

Loading

CodeBlanch Aug 15, 2020 •

edited

Loading

reyang Aug 15, 2020 •

edited

Loading

timr-dev Aug 15, 2020

ThomsonTan Aug 15, 2020

reyang Aug 16, 2020

ThomsonTan Aug 16, 2020

CodeBlanch left a comment

ThomsonTan commented Aug 16, 2020

ThomsonTan Aug 16, 2020

reyang Aug 16, 2020

reyang Aug 16, 2020

ThomsonTan Aug 16, 2020

reyang Aug 16, 2020

ThomsonTan Aug 16, 2020

ThomsonTan Aug 16, 2020

reyang Aug 16, 2020


		count = Math.Min(count, this.Count);

		for (int i = 0; i < count; i++)

Refactor exporter - step 4 #1085

Refactor exporter - step 4 #1085

Conversation

reyang commented Aug 15, 2020 • edited Loading

Changes

codecov bot commented Aug 15, 2020 • edited Loading

Codecov Report

CodeBlanch Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

reyang Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch left a comment

Choose a reason for hiding this comment

ThomsonTan commented Aug 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang commented Aug 15, 2020 •

edited

Loading

codecov bot commented Aug 15, 2020 •

edited

Loading

CodeBlanch Aug 15, 2020 •

edited

Loading

reyang Aug 15, 2020 •

edited

Loading