Add IMemoryPoolFactory and cleanup memory pool while idle

[BrennanConroy]

Adding the ability to release (to the GC) memory from the PinnedBlockMemoryPool. It does this by counting the rent and return calls and using heuristics to decide if it should free up memory or not based on pool usage.

Additionally implements the newly approved IMemoryPoolFactory<T> interface and plumbs it through Kestrel, IIS, and HttpSys with the default implementation for IMemoryPoolFactory<byte> being the PinnedBlockMemoryPool.

Kestrel makes use of its heartbeat loop to clean up the memory pools, whereas IIS and HttpSys don't have the equivalent so they use a PeriodicTimer approach.

Adds metrics to the pool. Issue for those is at #61594.

Closes #61003
Closes #27394
Closes #55490

Did a few perf runs to see the impact of Interlocked.Increment on the Rent and Return paths.
Did longer 60 second runs to make sure the memory pools had plenty of opportunity to run clean up logic and see the impact of incrementing counters.

• A few runs of Plaintext Platform for 60 seconds had no noticeable impact (this is our 24m RPS scenario so it should be hitting the pool a lot)
• A few runs of MvcJsonOutput60k for 60 seconds had no noticeable impact (wanted a benchmark that wrote data to the response)
• A few runs of Plaintext Platform with 2 IOQueues (default is # cores/2) to make more requests use the same pool, this one might have a tiny impact on RPS, but there seemed to be a bit of noise, was getting 5.6m and 5.7m with 2 queues and 5.8m and 6m without PR, but it seemed noisy.

TLDR; didn't notice a perf impact with this PR and so removed the ScalableApproximateCounting code for now. Can always add it back if we find problems.

[adityamandaleeka]

Do we want a metric for peak?

[BrennanConroy]

total_allocated covers that? Oh, nvm. Yeah, peak might be interesting.

[davidfowl]

Are we going to regret not making this configurable?

[davidfowl]

Maybe we should pass an options object so we can easily add future options

[JamesNK]

I'm not sure if these metrics should be on a new meter. Couldn't they be on a meter the same name as Kestrel's meter?

A new meter name means folks would need to configure OTEL to export the kestrel meter, and this System.Buffers.PinnedBlockMemoryPool meter. Having memory stats related to kestrel be on the Kestrel meter makes more sense to me.

[JamesNK]

I see there are multiple frameworks that use System.Buffers.PinnedBlockMemoryPool. Maybe it does make sense for it to be its own meter. Although if you have multiple frameworks using the pool at the same time then numbers will get merged together. Is that would you intend?

A solution would be to have an owner tag that says who used the memory. When a memory pool is created the owner could pass in their name, e.g. kestrel, sockets, iis, etc, and the name is internally passed to counters when they're changed. An owner tag would let you view total memory usage, vs just Kestrel HTTP layer, vs sockets layer, vs IIS (if running together in the same process)

[JamesNK]

I think the meter should be renamed to indicate that it's specific to ASP.NET Core. Maybe Microsoft.AspNetCore.MemoryPool? That is simple and obvious what the meter is for.

I don't think people care that the underlying pool is implemented using pinned blocks.

[JamesNK]

It looks like there could be multiple instances of memory pool used across assemblies with shared source. Fortunatly I believe that as long as the same IMeterFactory is used for all of them (injected by DI), the meter and counters created will be the same instances. Metrics values will be aggregated and saved to the same location.

However, I think a unit test to double check that is important.

[BrennanConroy]

Added a test and updated our TestMeterFactory implementation to return the same Meter instance for similar meters. Same as the default meter factory impl: https://source.dot.net/#Microsoft.Extensions.Diagnostics/Metrics/DefaultMeterFactory.cs,37

[JamesNK]

@lmolkova @noahfalk More metrics are being added to ASP.NET Core. If you have time, your feedback on naming and metrics usage would be valuable.

[JamesNK]

@BrennanConroy FYI naming guidelines for OTEL are here: https://opentelemetry.io/docs/specs/semconv/general/naming/

[davidfowl]

Here’s a review of the updated PinnedBlockMemoryPool from @dotnet/aspnetcore/pull/61554:

---

Key Changes and Features

• Idle Cleanup/Eviction:
  The pool now supports scheduled eviction of idle blocks using an adaptive algorithm based on recent rent/return activity.
• Eviction Scheduling:
  • TryScheduleEviction schedules an eviction every 10 seconds if needed.
  • Uses ThreadPool.UnsafeQueueUserWorkItem for eviction work.
• Eviction Algorithm (PerformEviction):
  • If activity is trending down, removes a percentage of blocks proportional to excess returns.
  • If activity is steady, removes 1% of the current pool (at least 1).
  • If no activity, removes 5% (at least 10) for aggressive idle trimming.
• Metrics:
  • Tracks metrics for memory usage, evictions, allocations, etc.
• Dispose Pattern:
  • Thread-safe with a lock and an optional disposal callback.
• Rent/Return:
  • Rent increments a counter, returns blocks to the pool unless disposed.
  • Rent over block size throws.
  • Double-increments rent count on allocation to bias the eviction algorithm.

---

Strengths

• Adaptive Trimming:
  The eviction logic is sophisticated and adapts to changing patterns of usage, which is excellent for minimizing memory waste and avoiding thrashing.
• Thread-Safe:
  Uses ConcurrentQueue and Interlocked for all counters and state, as well as a lock for Dispose.
• Metrics:
  Well-instrumented, which will help diagnose issues and tune performance.
• Activity-Aware:
  The algorithm takes recent rent/return activity into account, reducing the likelihood of evicting blocks that are needed soon.
• Clear Ownership Model:
  Internal comments and method signatures make it clear who owns buffers and how they must be returned.

---

Potential Issues / Suggestions

1. Eviction Calculation Granularity

   • When the pool is very small (say, less than 100 blocks), currentCount / 100 can be zero, so no blocks are evicted in “steady” or “trending less” activity. However, the Math.Max(1, ...) ensures at least 1 is evicted in the steady case.

2. Eviction Timer Drift

   • _nextEviction = now.AddSeconds(10);
     If eviction is delayed (ThreadPool busy), evictions may drift, but this is typical for such background cleanup and not a problem for a memory pool.

3. Dispose Race/Redundancy

   • The lock in Dispose(bool) is correct, but some state (_isDisposed) is set before the lock, which could allow a thread to see a partial dispose.
   • This risk is minor and would only affect metrics or rare edge cases. Consider moving _isDisposed = true; inside the lock for maximum safety.

4. Metrics Double-Counting

   • The logic for incrementing _rentCount twice on allocation is intentional (to reduce eviction under high allocation), but could slightly skew metrics for external consumers. This is a fair tradeoff for the intended behavior.

5. No Age-Based Eviction

   • All eviction is based on activity, not block idle age. This is typical and generally fine, but if age-based trimming is needed (e.g., only trim oldest), another queue or timestamps would be required.

6. Block Finalizer/Leak Recovery

   • The comments reference finalizer-based leak recovery, but this is not visible in this excerpt—assume the MemoryPoolBlock handles this as before.

7. Eviction During High Concurrency

   • If eviction is scheduled during a burst of high concurrency, there could be brief over-trimming. The activity-based scheme should minimize this risk, but monitoring is recommended in real workloads.

---

Summary Table

Aspect	Status	Notes
Thread Safety	✅	Uses concurrent structures and locking as needed
Adaptive Eviction	✅	Algorithm considers activity and trims efficiently
Metrics/Diagnostics	✅	Good instrumentation
Defensive Coding	✅	Careful checks on sizes, disposal, and ownership
Over/Under-trimming	⚠️/✅	Well-tuned, but as with all heuristics, workload-dependent
Disposal Robustness	✅	Locking and callback support

---

Conclusion

This is a robust, production-grade adaptive memory pool.

• The adaptive eviction is an excellent improvement over static trimming.
• Thread safety, metrics, and defensive checks are all strong.
• The only possible enhancement would be optional age-based eviction for even more precise memory usage control, but for most server workloads, this scheme is ideal.

Overall: 👍 Looks great and ready for real workloads.

[benaadams]

nit: Could ask @stephentoub to create a ConcurrentHashTable<T> 😉 or change the value to a recognised atomic type by ConcurrentDictionary e.g. nuint

    private readonly ConcurrentDictionary<PinnedBlockMemoryPool, nuint> _pools = new();

Will save on GC write barriers; thought probably not too important

[benaadams]

nit: here also ConcurrentDictionary value to nuint

ConcurrentDictionary<PinnedBlockMemoryPool, nuint>

[benaadams]

Better cancellation here? (e.g. throw away cancellation)

And log something more serious to ILogger at some level rather than Debug.WriteLine?