[SDK] Circular buffer tweaks + cpu pressure test #3349
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3349 +/- ##
==========================================
- Coverage 86.14% 85.63% -0.52%
==========================================
Files 268 268
Lines 9448 9444 -4
==========================================
- Hits 8139 8087 -52
- Misses 1309 1357 +48
|
|
@reyang @cijothomas Unless I misread or confused myself, I think all the feedback is addressed and this is ready to go 🤣 |
I need more time to review/test the change. |
|
|
||
| if (headSnapshot - tailSnapshot >= this.Capacity) | ||
| { | ||
| return false; // buffer is full | ||
| } | ||
|
|
||
| var head = Interlocked.CompareExchange(ref this.head, headSnapshot + 1, headSnapshot); | ||
| if (head != headSnapshot) | ||
| if (Interlocked.CompareExchange(ref this.head, headSnapshot + 1, headSnapshot) != headSnapshot) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
Would it be useful to add Spinwait.SpinOnce() here before continue?
There was a problem hiding this comment.
Defer to @reyang on this one, but I don't think so. This thread isn't so much waiting on another thread to finish as it is learning that some other thread took the head. It should retry immediately and just take the next head/index available.
There was a problem hiding this comment.
SpinOnce might be more smart yielding if singlecore etc?
There was a problem hiding this comment.
I think a yield here would be even worse than a spin 😄 Because it doesn't need to wait on anything. If there is only 1 core this thread should just loop around, take the next index, and continue on doing its thing. Same as if there were many cores, really.
There should probably be a SpinOnce here though:
Because that logic is actually waiting on the writer thread to finish. On single core, it should yield immediately because a spin won't accomplish anything other than delay letting the writer get the CPU back to finish its job 🤣
There was a problem hiding this comment.
I think a yield here would be even worse than a spin
+1
* Added Jaeger Propagator to Opentelemetry.Extensions.Propagators (#3309) * Remove unnecessary bullet in CHANGELOG.md (#3352) Co-authored-by: Cijo Thomas <[email protected]> * Fix OTLP test (#3357) * Show that test is not doing what you might think it does * More asserts the merrier * Show this little test that it has potential * improve test coverage: InMemoryExporter & IDeferredMeterProviderBuilder (#3345) * [SDK] Circular buffer tweaks + cpu pressure test (#3349) * CircularBuffer tweaks and cpu pressure test. * Switch to Volatile.Read. * Perf tweaks. * Remove race check in debug after doing more testing. Co-authored-by: Cijo Thomas <[email protected]> * Fix event name logic + support null categoryname. (#3359) * In-memory Exporter: Buffer log scopes (#3360) * Buffer log scopes when using in-memory exporter. * CHANGELOG update. * Code review. * Tests. * CHANGELOG tweak. * SDK: Forward SetParentProvider to children of CompositeProcessor (#3368) * Examples: Fix ParentProvider not being set on MyFilteringProcessor (#3370) * Fix ParentProvider not being set on MyFilteringProcessor example. * Added XML comments. * Tweak. * Typo. * Logs: Add helper ctors & forceflush on OpenTelemetryLoggerProvider (#3364) * Add helper ctors & forceflush on OpenTelemetryLoggerProvider. * CHANGELOG update. * Unit tests. * Code review. * Code review. * Tweak. * SDK: Nullable annotations for base classes & batch + shims to enable compiler features (#3374) * Nullable annotations and shims for SDK base classes & batch. * Target updates. * Remove System.Collections.Immutable ref. * ApiCompat attribute exclusions. * ASPNETCore instrumentation to populate httpflavor tag (#3372) * improve test coverage: InMemoryExporter SnapshotMetric (#3344) * Fix AspNetCore metrics to use correct value for http.flavor (#3379) * Fix AspNetCore metrics to use correct value for http.flavor * word better * Logs: Add LogRecordData (#3378) * Add LogRecordData and hook up to LogRecord. * CHANGELOG update. * Code review. * Remove SetHttpFlavor from Http instrumentations (#3381) * Asp.Net Core trace instrumentation to populate http schema tag (#3392) * Try asp.net core tests with inproc server (#3394) * Dedupe IsPackable (#3398) * Remove AspNet and AspNet.TelemetryHttpModule instrumentation projects (#3397) * Handle possible exception when initializing the default service name (#3405) * HttpClient: Invoke Enrich when SocketException = HostNotFound (#3407) * Add & use ConfigureResource API. (#3307) Co-authored-by: tyler jago <[email protected]> Co-authored-by: Robert Pająk <[email protected]> Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Timothy Mothra <[email protected]> Co-authored-by: Mikel Blanchard <[email protected]> Co-authored-by: Reiley Yang <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]> Co-authored-by: Christian Neumüller <[email protected]>
Changes
Ran into a race/deadlock using logic similar to CircularBuffer on #3305. Added a test to verify everything is good with CircularBuffer. In doing so I made a few tweaks:
Volatile.Read&Volatile.Writeso everything will work on 32bit processors.Interlocked.Exchangeto do atomic read + set to null in the reader.Also threw in nullable annotations since I was in there.