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.

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

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

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs

+        PerformEviction();
+    }
+
+    internal void PerformEviction()


Can you add a comment that explains how this works? Use AI to generate it 😄

davidfowl · 2025-05-15T08:27:07Z

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

+
+internal sealed class PinnedBlockMemoryPoolMetrics
+{
+    // Note: Dot separated instead of dash.


Is that uncommon?

@JamesNK

No, it's not uncommon for a meter name to use dots. All of our other meter names used. I think at one point the kestrel event source used dashes, but that's very old. Brennan might have copied that comment from there.

Comment doesn't seem nessessary

JamesNK · 2025-05-15T08:30:00Z

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

+
+internal sealed class PinnedBlockMemoryPoolMetrics
+{
+    // Note: Dot separated instead of dash.


No, it's not uncommon for a meter name to use dots. All of our other meter names used. I think at one point the kestrel event source used dashes, but that's very old. Brennan might have copied that comment from there.

Comment doesn't seem nessessary

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.

JamesNK · 2025-05-15T08:40:29Z

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

+        _meter = meterFactory.Create(MeterName);
+
+        _currentMemory = _meter.CreateUpDownCounter<long>(
+            "pinnedblockmemorypool.current_memory",


This name should probably be something like aspnetcore.memorypool.current_memory.

Use aspnetcore.memorypool.* as the prefix for counter names.

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

src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPoolMetrics.cs

+        _usageRate = _meter.CreateCounter<long>(
+            "pinnedblockmemorypool.usage_rate",
+            unit: "{bytes}",
+            description: "Rate of bytes rented from the pool."
+            );


How is this a rate?

That's what counters are. It just depends how you view the data.

https://learn.microsoft.com/dotnet/core/diagnostics/metrics-instrumentation#create-a-custom-metric

For Counter instruments, the convention is that collection tools show the total count and/or the rate at which the count is increasing

The runtime doesn't control whether it will be viewed as a rate so the counter name and description shouldn't presume that it will be. I'd suggest naming it based on the thing that is actually being counted. An example similar to this for total bytes allocated from the GC: RuntimeMetrics.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.

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.

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

JamesNK reviewed May 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IMemoryPoolFactory and cleanup memory pool while idle #61554

Add IMemoryPoolFactory and cleanup memory pool while idle #61554

BrennanConroy commented Apr 18, 2025 •

edited

Loading

adityamandaleeka May 15, 2025

BrennanConroy May 15, 2025 •

edited

Loading

davidfowl May 15, 2025

davidfowl May 15, 2025

davidfowl May 15, 2025

davidfowl May 15, 2025

JamesNK 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 15, 2025

JamesNK May 15, 2025 •

edited

Loading

JamesNK May 15, 2025

BrennanConroy May 20, 2025

noahfalk May 20, 2025

JamesNK May 15, 2025 •

edited

Loading

JamesNK commented May 15, 2025

JamesNK commented May 15, 2025

davidfowl commented May 18, 2025

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

Conversation

BrennanConroy commented Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

BrennanConroy May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK May 15, 2025 • edited Loading

Choose a reason for hiding this comment

JamesNK May 15, 2025 • edited Loading

Choose a reason for hiding this comment

JamesNK May 15, 2025 • edited Loading

Choose a reason for hiding this comment

JamesNK May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK May 15, 2025 • edited Loading

Choose a reason for hiding this comment

JamesNK commented May 15, 2025

JamesNK commented May 15, 2025

davidfowl commented May 18, 2025

Key Changes and Features

Strengths

Potential Issues / Suggestions

Summary Table

Conclusion

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 15, 2025 •

edited

Loading

JamesNK May 15, 2025 •

edited

Loading

JamesNK May 15, 2025 •

edited

Loading