Add IMemoryPoolFactory and cleanup memory pool while idle #61554

BrennanConroy · 2025-04-18T21:34:10Z

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.

src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs

src/Shared/Buffers.MemoryPool/DefaultMemoryPoolFactory.cs

adityamandaleeka · 2025-05-15T04:54:38Z

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

+    private readonly Counter<long> _evictedMemory;
+    private readonly Counter<long> _usageRate;
+
+    public PinnedBlockMemoryPoolMetrics(IMeterFactory meterFactory)


Do we want a metric for peak?

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

I think peak is just measuring the max of current_memory?

davidfowl · 2025-05-15T08:25:45Z

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs

+
+    private long _currentMemory;
+    private long _evictedMemory;
+    private DateTimeOffset _nextEviction = DateTime.UtcNow.AddSeconds(10);


Are we going to regret not making this configurable?

davidfowl · 2025-05-15T08:26:12Z

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs

+    {
+    }
+
+    public PinnedBlockMemoryPool(IMeterFactory meterFactory)


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

How would you set the options? Just have a generic MemoryPoolOptions type that users can configure in DI? That sounds like something we could add in the future without breaking anything.

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

JamesNK · 2025-05-15T08:32:22Z

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

+internal sealed class PinnedBlockMemoryPoolMetrics
+{
+    // Note: Dot separated instead of dash.
+    public const string MeterName = "System.Buffers.PinnedBlockMemoryPool";


~~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.

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)

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.

Changed the name. But can we limit metric feedback to the issue #61594?

The owner tag seems interesting, but how would it actually work? For example, the app layer can grab the memory pool factory via DI and get a pool instance, so if you wanted a different name for app code then the interface would need to allow setting a name. If this is something you think we should pursue, can you comment on the issue I linked?

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

JamesNK · 2025-05-15T08:47:17Z

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

+
+    public PinnedBlockMemoryPoolMetrics(IMeterFactory meterFactory)
+    {
+        _meter = meterFactory.Create(MeterName);


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.

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 · 2025-05-15T08:48:40Z

@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 · 2025-05-15T08:52:04Z

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

davidfowl · 2025-05-18T23:57:57Z

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

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.
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.
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.
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.
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.
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.
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 · 2025-05-23T05:08:29Z

src/Servers/Kestrel/Core/src/Internal/PinnedBlockMemoryPoolFactory.cs

+{
+    private readonly IMeterFactory _meterFactory;
+    private readonly TimeProvider _timeProvider;
+    private readonly ConcurrentDictionary<PinnedBlockMemoryPool, PinnedBlockMemoryPool> _pools = new();


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

Link to any documentation or discussion about this?

Perhaps: https://learn.microsoft.com/en-us/previous-versions/dotnet/articles/ms973837(v=msdn.10)?redirectedfrom=MSDN#making-generations-work-with-write-barriers
https://learn.microsoft.com/en-us/previous-versions/dotnet/articles/ms973837(v=msdn.10)?redirectedfrom=MSDN#too-many-object-writes

A short version is that updating a reference-typed field is a little more expensive than a value-typed field. Its a micro-optimization.

Micro-optimization on the correct data structure not being available in runtime (as both key and value are always the same and value never read)

benaadams · 2025-05-23T05:15:47Z

src/Shared/Buffers.MemoryPool/DefaultMemoryPoolFactory.cs

+internal sealed class DefaultMemoryPoolFactory : IMemoryPoolFactory<byte>, IAsyncDisposable
+{
+    private readonly IMeterFactory _meterFactory;
+    private readonly ConcurrentDictionary<PinnedBlockMemoryPool, PinnedBlockMemoryPool> _pools = new();


nit: here also ConcurrentDictionary value to nuint

ConcurrentDictionary<PinnedBlockMemoryPool, nuint>

src/Shared/Buffers.MemoryPool/DefaultMemoryPoolFactory.cs

src/Servers/HttpSys/src/HttpSysListener.cs

JamesNK · 2025-05-29T02:52:44Z

src/Shared/Buffers.MemoryPool/DefaultMemoryPoolFactory.cs

+
+    public DefaultMemoryPoolFactory(IMeterFactory? meterFactory = null)
+    {
+        _meterFactory = meterFactory ?? NoopMeterFactory.Instance;


This looks like it would only be null in tests? I think it would be better if the type always expect a meter factory from DI. And then test code is responsible for creating the factory with a test meter factory. Then NoopMeterFactory can be remove from this type and https://github.com/dotnet/aspnetcore/blob/2ad1ff0a9e870a77522fc0725153154834bcb0ac/src/Shared/Metrics/TestMeterFactory.cs used when this type is created in tests.

JamesNK · 2025-05-29T02:55:10Z

src/Shared/Buffers.MemoryPool/DefaultMemoryPoolFactory.cs

+        await _timerTask;
+    }
+
+    private sealed class NoopMeterFactory : IMeterFactory


This name isn't accurate because the factory will still create meters and people can still listen to those meters. The only difference is the factory isn't set as the owner of the meters. In other words, they're harder to test.

A better simulation of "no op" would be to not create PinnedBlockMemoryPoolMetrics if there's no meter factory and any calls to it would be _metrics?.UpdateCurrentMemory(-block.Memory.Length);

As mentioned here, I think the best improvement to make here is always require the meter factory and have tests always specify it.

I went with allowing a null IMeterFactory. Most tests aren't doing anything with meters, so it seems like a waste to require them to pass in a meter factory.

BrennanConroy added 11 commits February 28, 2025 11:30

wip

6ba4ffb

stash

de7c126

IMemoryPoolFactory

3a1b780

cleanup

c4f250a

rebase

d44c3af

wip

64cbbee

namedpipe

f17e3eb

api review

a1c262c

progress

206542e

stash

bd9cd25

iis

9ae6a5b

BrennanConroy added feature-iis Includes: IIS, ANCM feature-kestrel feature-httpsys area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Apr 18, 2025

davidfowl reviewed Apr 18, 2025

View reviewed changes

src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Apr 19, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

davidfowl reviewed Apr 19, 2025

View reviewed changes

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs Outdated Show resolved Hide resolved

davidfowl reviewed Apr 19, 2025

View reviewed changes

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs Outdated Show resolved Hide resolved

davidfowl reviewed Apr 19, 2025

View reviewed changes

src/Shared/Buffers.MemoryPool/DefaultMemoryPoolFactory.cs Outdated Show resolved Hide resolved

davidfowl reviewed Apr 19, 2025

View reviewed changes

src/Shared/Buffers.MemoryPool/DefaultMemoryPoolFactory.cs Outdated Show resolved Hide resolved

fb

fdfaad5

adityamandaleeka mentioned this pull request May 2, 2025

Allow server Memory Pool to shrink #27394

Open

adityamandaleeka linked an issue May 2, 2025 that may be closed by this pull request

Allow server Memory Pool to shrink #27394

Open

cleanup

815e397

BrennanConroy marked this pull request as ready for review May 15, 2025 01:13

BrennanConroy requested review from halter73, JamesNK and mgravell as code owners May 15, 2025 01:13

BrennanConroy requested a review from adityamandaleeka May 15, 2025 01:13

BrennanConroy requested review from captainsafia and DeagleGross May 15, 2025 01:13

build-analysis bot mentioned this pull request May 15, 2025

System.TimeoutException : The operation has timed out. dotnet/dnceng#5279

Open

3 tasks

adityamandaleeka reviewed May 15, 2025

View reviewed changes

davidfowl reviewed May 15, 2025

View reviewed changes

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs Show resolved Hide resolved

davidfowl reviewed May 15, 2025

View reviewed changes

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs Outdated Show resolved Hide resolved

JamesNK reviewed May 15, 2025

View reviewed changes

Naming and Metrics tests

b903a1f

benaadams reviewed May 23, 2025

View reviewed changes

JamesNK reviewed May 29, 2025

View reviewed changes

BrennanConroy mentioned this pull request May 29, 2025

[API Proposal]: PinnedBlockMemoryPool metrics #61594

Open

fb

35cac4e

build-analysis bot mentioned this pull request May 29, 2025

Known Build Error: error : Cannot create a file when that file already exists. #61178

Open

Add IMemoryPoolFactory and cleanup memory pool while idle #61554

Are you sure you want to change the base?

Add IMemoryPoolFactory and cleanup memory pool while idle #61554

Uh oh!

Conversation

BrennanConroy commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesNK May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JamesNK May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK commented May 15, 2025

Uh oh!

JamesNK commented May 15, 2025

Uh oh!

davidfowl commented May 18, 2025

Key Changes and Features

Strengths

Potential Issues / Suggestions

Summary Table

Conclusion

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JamesNK May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

BrennanConroy commented Apr 18, 2025 •

edited

Loading

BrennanConroy May 15, 2025 •

edited

Loading

JamesNK May 15, 2025 •

edited

Loading

JamesNK May 15, 2025 •

edited

Loading

JamesNK May 15, 2025 •

edited

Loading

JamesNK May 29, 2025 •

edited

Loading

JamesNK May 29, 2025 •

edited

Loading