[OTLP] Use ConditionalWeakTable for cache#7326
Conversation
Follow-up to open-telemetry#7303 to use `ConditionalWeakTable<K, V>` instead of `ConcurrentDictionary`<K, V>.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7326 +/- ##
==========================================
+ Coverage 89.86% 89.90% +0.03%
==========================================
Files 276 276
Lines 14006 14007 +1
==========================================
+ Hits 12587 12593 +6
+ Misses 1419 1414 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Pull request overview
Updates the OTLP protobuf resource serialization cache to avoid retaining Resource instances indefinitely by switching from a ConcurrentDictionary to a ConditionalWeakTable, allowing cached entries to be reclaimed when the Resource key is no longer referenced.
Changes:
- Replaced the resource-bytes cache from
ConcurrentDictionary<Resource, byte[]>toConditionalWeakTable<Resource, byte[]>. - Added framework-conditional cache access (
GetOrAddonnet10.0+,GetValueotherwise).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cijothomas
left a comment
There was a problem hiding this comment.
Thanks. If possible, lets add a test to validate that we don't hold up Resource, if this is easy to test.
Add a unit test that verifies the resource isn't kept alive in the cache.
#7303 (comment)
Changes
Follow-up to #7303 to use
ConditionalWeakTable<K, V>instead ofConcurrentDictionary<K, V>.@cijothomas does this resolve your concern in the other PR?
Benchmarks
While this PR degrades compared to the original improvements, it is still significantly faster than before the caching was added.
Run with:
Copilot Summaries
This PR. vs main
gh-7326 is a mixed bag on .NET 10.0 but a clear regression on .NET Framework 4.6.2. Allocations are unchanged at 0 B in every benchmark, so allocation deltas are 0 B throughout and allocation ratios vs. baseline are n/a because the baseline is zero.
Duration vs. baseline (
gh-7326 / main)High-level summary
Allocations
This PR vs. Before 7303
gh-7326 is substantially faster than baseline in 9 of 10 cases, with unchanged allocations. The only regression is .NET Framework 4.6.2, AttributeCount=0, which is 1.40x the baseline duration. All allocation measurements are 0 B vs 0 B, so allocation ratio is 1.00x across the board.
Duration ratio =
gh-7326 / baseline; lower than1.00xis better.Allocation summary
Overall: .NET 10.0 improved across every case, and .NET Framework 4.6.2 improved sharply once AttributeCount >= 4.
Before 7303 2435127
main (34dcd84)
This PR
Merge requirement checklist
Unit tests added/updatedAppropriateCHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)