DataDog
Memory Leak
Describe the bug
Memory leak due to [ThreadStatic] when no synchronization context is present.
To Reproduce Steps to reproduce the behavior:
- Enable tracer for any long running ASP.NET Core application with a large threadpool, or high threadpool churn Due to continuations running on any threadpool thread:
- The cached
StringBuildercan be returned to any thread - The thread static memory persists longer than expected (Possibly forever).
Expected behavior Stable memory usage
Screenshots N/A
Runtime environment (please complete the following information):
- Instrumentation mode: CLR profiler on linux
- Tracer version: 2.1.1
- OS: Cent OS 8.2
- CLR: .NET 6.0
Additional context
The use of [ThreadStatic] in StringBuilderCache results in a memory leak with high thread pool worker counts, or churn.
We have several high load/performance applications that override the default threadpool configuration, and in each of them StringBuilder uses an excessive amount of memory.
Pooling with very short lived locks should be used in place of [ThreadStatic]
I will submit a PR to replace this code with a pooling mechanism instead.
Thanks for reaching out, Jason. StringBuilderCache is based on this internal implementation in .NET:
https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/System/Text/StringBuilderCache.cs
Its goal is to avoid allocating (and collecting) many small StringBuilder instances when we could be reusing the same instance multiple times. Since the cached instance are only used within the same thread, no locking is required.
It's a trade-off between "reserving" more heap memory vs allocating (and collecting) it every time we need it. The maximum size of a cached StringBuilder is 360 (so ~720 bytes in a char[] plus another ~20 bytes in some int fields and such).
...results in a memory leak with high thread pool worker counts... ...StringBuilder uses an excessive amount of memory...
To better understand and possibly replicate your scenario, approximately how many thread pool threads are we talking about? And how much memory is StringBuilder using?
Before you do the work to submit a PR with a pooling mechanism, I'm wondering if a simpler knob would help here, like a flag to disable this caching by always returning a new StringBuilder in StringBuilderCache.Acquire (and making GetStringAndRelease() a no-op).
(edit: typos) (edit: 160 bits => 20 bytes)
Its goal is to avoid allocating (and collecting) many small StringBuilder instances when we could be reusing the same instance multiple times. Since the cached instance are only used within the same thread, no locking is required.
It's a tread off between "reserving" more heap memory vs allocating it every time we need it. The maximum size of a cached StringBuilder is 360 (so ~720 bytes in a char[] plus another ~160 bytes in some int fields and such). ... To better understand and possibly replicate your scenario, approximately how many thread pool threads are we talking about? And how much memory is StringBuilder using?
Understood. We're seeing about 40mb across 65535 thread pool threads. Admittedly the application is really the problem, but a pooling mechanism, even with locks, can avoid allocating so many string builders. At the cost of a very short lived lock (Should probably be a spinwait instead though).
Before you do the work to submit a PR with a pooling mechanism, I'm wondering if a simpler knob would help here, like a flag to disable this caching by always returning a new StringBuilder in StringBuilderCache.Acquire (and making GetStringAndRelease() a no-op).
This makes a lot more sense, but I already submitted the PR. I'll happily do that method tomorrow and submit another one.