Add global Stream tracker in debug builds #5598
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5598 +/- ##
==========================================
+ Coverage 84.81% 85.29% +0.47%
==========================================
Files 60 60
Lines 18647 18658 +11
==========================================
+ Hits 15816 15914 +98
+ Misses 2831 2744 -87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| CxPlatLockInitialize(&MsQuicLib.Lock); | ||
| CxPlatDispatchLockInitialize(&MsQuicLib.DatapathLock); | ||
| #if DEBUG | ||
| CxPlatDispatchLockInitialize(&QuicStreamTrackerLock); |
There was a problem hiding this comment.
Nit: I'd suggest simplifying this to one lock for all the trackers.
| // | ||
| // Global stream object tracker for debugging. | ||
| // | ||
| CXPLAT_LIST_ENTRY QuicStreamTrackerList = { &QuicStreamTrackerList, &QuicStreamTrackerList }; |
There was a problem hiding this comment.
I've never seen this pattern before - is there a precedent for it in MsQuic? If not, let's use the init routine.
| #if DEBUG | ||
| CXPLAT_DBG_ASSERT(!CxPlatRefDecrement(&Stream->RefTypeBiasedCount[QUIC_STREAM_REF_APP])); | ||
| CxPlatDispatchLockAcquire(&QuicStreamTrackerLock); | ||
| CxPlatListEntryRemove(&Stream->TrackerLink); |
There was a problem hiding this comment.
I think we may want to introduce some abstractions for these trackers, such that if perf becomes a practical bottleneck on debug builds, we can:
- Easily add some more fields beyond the list entry
- Easily replace the single global list with something more sophisticated, such as a per-CPU list
|
I am not convinced this helps given that |
Description
Help debug lost reference issues in MsQuic by keeping a global list of streams.
Testing
CI
Documentation
N/A