-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add IMemoryPoolFactory and cleanup memory pool while idle #61554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6ba4ffb
de7c126
3a1b780
c4f250a
d44c3af
64cbbee
f17e3eb
a1c262c
206542e
bd9cd25
9ae6a5b
fdfaad5
815e397
b903a1f
35cac4e
aa7f1d0
990c396
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Buffers; | ||
|
||
namespace Microsoft.AspNetCore.Connections; | ||
|
||
/// <summary> | ||
/// Interface for creating memory pools. | ||
/// </summary> | ||
public interface IMemoryPoolFactory<T> | ||
{ | ||
/// <summary> | ||
/// Creates a new instance of a memory pool. | ||
/// </summary> | ||
/// <returns>A new memory pool instance.</returns> | ||
MemoryPool<T> Create(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
#nullable enable | ||
Microsoft.AspNetCore.Connections.IMemoryPoolFactory<T> | ||
Microsoft.AspNetCore.Connections.IMemoryPoolFactory<T>.Create() -> System.Buffers.MemoryPool<T>! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
#nullable enable | ||
Microsoft.AspNetCore.Connections.IMemoryPoolFactory<T> | ||
Microsoft.AspNetCore.Connections.IMemoryPoolFactory<T>.Create() -> System.Buffers.MemoryPool<T>! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
#nullable enable | ||
Microsoft.AspNetCore.Connections.IMemoryPoolFactory<T> | ||
Microsoft.AspNetCore.Connections.IMemoryPoolFactory<T>.Create() -> System.Buffers.MemoryPool<T>! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
#nullable enable | ||
Microsoft.AspNetCore.Connections.IMemoryPoolFactory<T> | ||
Microsoft.AspNetCore.Connections.IMemoryPoolFactory<T>.Create() -> System.Buffers.MemoryPool<T>! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Buffers; | ||
using System.Collections.Concurrent; | ||
using System.Diagnostics.Metrics; | ||
using Microsoft.AspNetCore.Connections; | ||
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; | ||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal; | ||
|
||
internal sealed class PinnedBlockMemoryPoolFactory : IMemoryPoolFactory<byte>, IHeartbeatHandler | ||
{ | ||
private readonly IMeterFactory _meterFactory; | ||
private readonly ILogger? _logger; | ||
private readonly TimeProvider _timeProvider; | ||
// micro-optimization: Using nuint as the value type to avoid GC write barriers; could replace with ConcurrentHashSet if that becomes available | ||
private readonly ConcurrentDictionary<PinnedBlockMemoryPool, nuint> _pools = new(); | ||
|
||
public PinnedBlockMemoryPoolFactory(IMeterFactory meterFactory, TimeProvider? timeProvider = null, ILogger<PinnedBlockMemoryPoolFactory>? logger = null) | ||
{ | ||
_timeProvider = timeProvider ?? TimeProvider.System; | ||
_meterFactory = meterFactory; | ||
_logger = logger; | ||
} | ||
|
||
public MemoryPool<byte> Create() | ||
{ | ||
var pool = new PinnedBlockMemoryPool(_meterFactory, _logger); | ||
|
||
_pools.TryAdd(pool, nuint.Zero); | ||
|
||
pool.OnPoolDisposed(static (state, self) => | ||
{ | ||
((ConcurrentDictionary<PinnedBlockMemoryPool, nuint>)state!).TryRemove(self, out _); | ||
}, _pools); | ||
|
||
return pool; | ||
} | ||
|
||
public void OnHeartbeat() | ||
{ | ||
var now = _timeProvider.GetUtcNow(); | ||
foreach (var pool in _pools) | ||
{ | ||
pool.Key.TryScheduleEviction(now); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Buffers; | ||
using System.Collections.Concurrent; | ||
using System.Reflection; | ||
using Microsoft.AspNetCore.InternalTesting; | ||
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal; | ||
using Microsoft.Extensions.Time.Testing; | ||
|
||
namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests; | ||
|
||
public class PinnedBlockMemoryPoolFactoryTests | ||
{ | ||
[Fact] | ||
public void CreatePool() | ||
{ | ||
var factory = new PinnedBlockMemoryPoolFactory(new TestMeterFactory()); | ||
var pool = factory.Create(); | ||
Assert.NotNull(pool); | ||
Assert.IsType<PinnedBlockMemoryPool>(pool); | ||
} | ||
|
||
[Fact] | ||
public void CreateMultiplePools() | ||
{ | ||
var factory = new PinnedBlockMemoryPoolFactory(new TestMeterFactory()); | ||
var pool1 = factory.Create(); | ||
var pool2 = factory.Create(); | ||
|
||
Assert.NotNull(pool1); | ||
Assert.NotNull(pool2); | ||
Assert.NotSame(pool1, pool2); | ||
} | ||
|
||
[Fact] | ||
public void DisposePoolRemovesFromFactory() | ||
{ | ||
var factory = new PinnedBlockMemoryPoolFactory(new TestMeterFactory()); | ||
var pool = factory.Create(); | ||
Assert.NotNull(pool); | ||
|
||
var dict = (ConcurrentDictionary<PinnedBlockMemoryPool, nuint>)(typeof(PinnedBlockMemoryPoolFactory) | ||
.GetField("_pools", BindingFlags.NonPublic | BindingFlags.Instance) | ||
?.GetValue(factory)); | ||
Assert.Single(dict); | ||
|
||
pool.Dispose(); | ||
Assert.Empty(dict); | ||
} | ||
|
||
[Fact] | ||
public async Task FactoryHeartbeatWorks() | ||
{ | ||
var timeProvider = new FakeTimeProvider(DateTimeOffset.UtcNow.AddDays(1)); | ||
var factory = new PinnedBlockMemoryPoolFactory(new TestMeterFactory(), timeProvider); | ||
|
||
// Use 2 pools to make sure they all get triggered by the heartbeat | ||
var pool = Assert.IsType<PinnedBlockMemoryPool>(factory.Create()); | ||
var pool2 = Assert.IsType<PinnedBlockMemoryPool>(factory.Create()); | ||
|
||
var blocks = new List<IMemoryOwner<byte>>(); | ||
for (var i = 0; i < 10000; i++) | ||
{ | ||
blocks.Add(pool.Rent()); | ||
blocks.Add(pool2.Rent()); | ||
} | ||
|
||
foreach (var block in blocks) | ||
{ | ||
block.Dispose(); | ||
} | ||
blocks.Clear(); | ||
|
||
// First eviction pass likely won't do anything since the pool was just very active | ||
factory.OnHeartbeat(); | ||
|
||
var previousCount = pool.BlockCount(); | ||
var previousCount2 = pool2.BlockCount(); | ||
timeProvider.Advance(TimeSpan.FromSeconds(10)); | ||
factory.OnHeartbeat(); | ||
|
||
await VerifyPoolEviction(pool, previousCount); | ||
await VerifyPoolEviction(pool2, previousCount2); | ||
|
||
timeProvider.Advance(TimeSpan.FromSeconds(10)); | ||
|
||
previousCount = pool.BlockCount(); | ||
previousCount2 = pool2.BlockCount(); | ||
factory.OnHeartbeat(); | ||
|
||
await VerifyPoolEviction(pool, previousCount); | ||
await VerifyPoolEviction(pool2, previousCount2); | ||
|
||
static async Task VerifyPoolEviction(PinnedBlockMemoryPool pool, int previousCount) | ||
{ | ||
// Because the eviction happens on a thread pool thread, we need to wait for it to complete | ||
// and the only way to do that (without adding a test hook in the pool code) is to delay. | ||
// But we don't want to add an arbitrary delay, so we do a short delay with block count checks | ||
// to reduce the wait time. | ||
var maxWait = TimeSpan.FromSeconds(5); | ||
while (pool.BlockCount() > previousCount - (previousCount / 30) && maxWait > TimeSpan.Zero) | ||
{ | ||
await Task.Delay(50); | ||
maxWait -= TimeSpan.FromMilliseconds(50); | ||
} | ||
|
||
// Assert that the block count has decreased by 3.3-10%. | ||
// This relies on the current implementation of eviction logic which may change in the future. | ||
Assert.InRange(pool.BlockCount(), previousCount - (previousCount / 10), previousCount - (previousCount / 30)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,10 @@ | |
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using Microsoft.AspNetCore.Connections; | ||
using Microsoft.AspNetCore.Server.Kestrel.Internal; | ||
using Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.DependencyInjection.Extensions; | ||
|
||
namespace Microsoft.AspNetCore.Hosting; | ||
|
||
|
@@ -25,7 +27,14 @@ public static IWebHostBuilder UseSockets(this IWebHostBuilder hostBuilder) | |
{ | ||
return hostBuilder.ConfigureServices(services => | ||
{ | ||
services.AddSingleton<IConnectionListenerFactory, SocketTransportFactory>(); | ||
services.TryAddSingleton<IConnectionListenerFactory, SocketTransportFactory>(); | ||
|
||
services.TryAddSingleton<IMemoryPoolFactory<byte>, DefaultSimpleMemoryPoolFactory>(); | ||
services.AddOptions<SocketTransportOptions>().Configure((SocketTransportOptions options, IMemoryPoolFactory<byte> factory) => | ||
{ | ||
// Set the IMemoryPoolFactory from DI on SocketTransportOptions. Usually this should be the PinnedBlockMemoryPoolFactory from UseKestrelCore. | ||
options.MemoryPoolFactory = factory; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we're changing the type of the MemoryPoolFactory from a Func to an IMemoryPoolFactory meaning we're breaking reflection anyway, why flow the factory through the options instead of constructor injecting it in SocketTransportFactory? Just to minimize churn? I guess this also ensures anyone constructing it themselves with an options instance resolved from DI gets the appropriate IMemoryPoolFactory. I think it's fine to leave it, but I think it's our last chance not to flow the factory through options if we don't want to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would mean new API (since the type is public) or using the internal impl trick and switching to that. We'd need to do it on NamedPipes as well. |
||
}); | ||
}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Buffers; | ||
using Microsoft.AspNetCore.Connections; | ||
|
||
namespace Microsoft.AspNetCore.Server.Kestrel.Internal; | ||
|
||
internal sealed class DefaultSimpleMemoryPoolFactory : IMemoryPoolFactory<byte> | ||
{ | ||
public static DefaultSimpleMemoryPoolFactory Instance { get; } = new DefaultSimpleMemoryPoolFactory(); | ||
|
||
public MemoryPool<byte> Create() | ||
{ | ||
return MemoryPool<byte>.Shared; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,22 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Buffers; | ||
using Microsoft.AspNetCore.Hosting; | ||
using Microsoft.Extensions.Hosting; | ||
|
||
namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests; | ||
|
||
public static class TransportSelector | ||
{ | ||
public static IHostBuilder GetHostBuilder(Func<MemoryPool<byte>> memoryPoolFactory = null, | ||
long? maxReadBufferSize = null) | ||
public static IHostBuilder GetHostBuilder(long? maxReadBufferSize = null) | ||
{ | ||
return new HostBuilder() | ||
.ConfigureWebHost(webHostBuilder => | ||
{ | ||
webHostBuilder | ||
.UseSockets(options => | ||
{ | ||
options.MemoryPoolFactory = memoryPoolFactory ?? options.MemoryPoolFactory; | ||
options.MaxReadBufferSize = maxReadBufferSize; | ||
}); | ||
webHostBuilder.UseSockets(options => | ||
{ | ||
options.MaxReadBufferSize = maxReadBufferSize; | ||
}); | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Buffers; | ||
using System.Collections.Concurrent; | ||
using System.Diagnostics; | ||
using System.Diagnostics.Metrics; | ||
using Microsoft.AspNetCore.Connections; | ||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Microsoft.AspNetCore; | ||
|
||
#nullable enable | ||
|
||
internal sealed class DefaultMemoryPoolFactory : IMemoryPoolFactory<byte>, IAsyncDisposable | ||
{ | ||
private readonly IMeterFactory? _meterFactory; | ||
private readonly ConcurrentDictionary<PinnedBlockMemoryPool, bool> _pools = new(); | ||
private readonly PeriodicTimer _timer; | ||
private readonly Task _timerTask; | ||
private readonly ILogger? _logger; | ||
|
||
public DefaultMemoryPoolFactory(IMeterFactory? meterFactory = null, ILogger<DefaultMemoryPoolFactory>? logger = null) | ||
{ | ||
_meterFactory = meterFactory; | ||
_logger = logger; | ||
_timer = new PeriodicTimer(PinnedBlockMemoryPool.DefaultEvictionDelay); | ||
_timerTask = Task.Run(async () => | ||
{ | ||
try | ||
{ | ||
while (await _timer.WaitForNextTickAsync()) | ||
{ | ||
foreach (var pool in _pools.Keys) | ||
{ | ||
pool.PerformEviction(); | ||
} | ||
} | ||
} | ||
catch (Exception ex) | ||
{ | ||
_logger?.LogCritical(ex, "Error while evicting memory from pools."); | ||
} | ||
}); | ||
} | ||
|
||
public MemoryPool<byte> Create() | ||
{ | ||
var pool = new PinnedBlockMemoryPool(_meterFactory, _logger); | ||
|
||
_pools.TryAdd(pool, true); | ||
|
||
pool.OnPoolDisposed(static (state, self) => | ||
{ | ||
((ConcurrentDictionary<PinnedBlockMemoryPool, bool>)state!).TryRemove(self, out _); | ||
}, _pools); | ||
|
||
return pool; | ||
} | ||
|
||
public async ValueTask DisposeAsync() | ||
{ | ||
_timer.Dispose(); | ||
await _timerTask; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,26 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace System.Buffers; | ||
using System.Buffers; | ||
using System.Diagnostics.Metrics; | ||
|
||
internal static class PinnedBlockMemoryPoolFactory | ||
namespace Microsoft.AspNetCore; | ||
|
||
#nullable enable | ||
|
||
internal static class TestMemoryPoolFactory | ||
{ | ||
public static MemoryPool<byte> Create() | ||
public static MemoryPool<byte> Create(IMeterFactory? meterFactory = null) | ||
{ | ||
#if DEBUG | ||
return new DiagnosticMemoryPool(CreatePinnedBlockMemoryPool()); | ||
return new DiagnosticMemoryPool(CreatePinnedBlockMemoryPool(meterFactory)); | ||
#else | ||
return CreatePinnedBlockMemoryPool(); | ||
return CreatePinnedBlockMemoryPool(meterFactory); | ||
#endif | ||
} | ||
|
||
public static MemoryPool<byte> CreatePinnedBlockMemoryPool() | ||
public static MemoryPool<byte> CreatePinnedBlockMemoryPool(IMeterFactory? meterFactory = null) | ||
{ | ||
return new PinnedBlockMemoryPool(); | ||
return new PinnedBlockMemoryPool(meterFactory); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,22 +1,31 @@ | ||||||
// Licensed to the .NET Foundation under one or more agreements. | ||||||
// The .NET Foundation licenses this file to you under the MIT license. | ||||||
|
||||||
using System.Buffers; | ||||||
using System.Collections.Concurrent; | ||||||
using System.Diagnostics.Metrics; | ||||||
using Microsoft.Extensions.Logging; | ||||||
|
||||||
#nullable enable | ||||||
|
||||||
namespace System.Buffers; | ||||||
namespace Microsoft.AspNetCore; | ||||||
|
||||||
/// <summary> | ||||||
/// Used to allocate and distribute re-usable blocks of memory. | ||||||
/// </summary> | ||||||
internal sealed class PinnedBlockMemoryPool : MemoryPool<byte> | ||||||
internal sealed class PinnedBlockMemoryPool : MemoryPool<byte>, IThreadPoolWorkItem | ||||||
{ | ||||||
/// <summary> | ||||||
/// The size of a block. 4096 is chosen because most operating systems use 4k pages. | ||||||
/// </summary> | ||||||
private const int _blockSize = 4096; | ||||||
|
||||||
// 10 seconds chosen arbitrarily. Trying to avoid running eviction too frequently | ||||||
// to avoid trashing if the server gets batches of requests, but want to run often | ||||||
// enough to avoid memory bloat if the server is idle for a while. | ||||||
// This can be tuned later if needed. | ||||||
public static readonly TimeSpan DefaultEvictionDelay = TimeSpan.FromSeconds(10); | ||||||
|
||||||
/// <summary> | ||||||
/// Max allocation block size for pooled blocks, | ||||||
/// larger values can be leased but they will be disposed after use rather than returned to the pool. | ||||||
|
@@ -39,13 +48,41 @@ internal sealed class PinnedBlockMemoryPool : MemoryPool<byte> | |||||
/// </summary> | ||||||
private bool _isDisposed; // To detect redundant calls | ||||||
|
||||||
private readonly PinnedBlockMemoryPoolMetrics? _metrics; | ||||||
private readonly ILogger? _logger; | ||||||
|
||||||
private long _currentMemory; | ||||||
private long _evictedMemory; | ||||||
private DateTimeOffset _nextEviction = DateTime.UtcNow.Add(DefaultEvictionDelay); | ||||||
|
||||||
private uint _rentCount; | ||||||
private uint _returnCount; | ||||||
|
||||||
private readonly object _disposeSync = new object(); | ||||||
|
||||||
private Action<object?, PinnedBlockMemoryPool>? _onPoolDisposed; | ||||||
private object? _onPoolDisposedState; | ||||||
|
||||||
/// <summary> | ||||||
/// This default value passed in to Rent to use the default value for the pool. | ||||||
/// </summary> | ||||||
private const int AnySize = -1; | ||||||
|
||||||
public PinnedBlockMemoryPool(IMeterFactory? meterFactory = null, ILogger? logger = null) | ||||||
{ | ||||||
_metrics = meterFactory is null ? null : new PinnedBlockMemoryPoolMetrics(meterFactory); | ||||||
_logger = logger; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Register a callback to call when the pool is being disposed. | ||||||
/// </summary> | ||||||
public void OnPoolDisposed(Action<object?, PinnedBlockMemoryPool> onPoolDisposed, object? state = null) | ||||||
{ | ||||||
_onPoolDisposed = onPoolDisposed; | ||||||
_onPoolDisposedState = state; | ||||||
} | ||||||
|
||||||
public override IMemoryOwner<byte> Rent(int size = AnySize) | ||||||
{ | ||||||
if (size > _blockSize) | ||||||
|
@@ -58,11 +95,26 @@ public override IMemoryOwner<byte> Rent(int size = AnySize) | |||||
MemoryPoolThrowHelper.ThrowObjectDisposedException(MemoryPoolThrowHelper.ExceptionArgument.MemoryPool); | ||||||
} | ||||||
|
||||||
Interlocked.Increment(ref _rentCount); | ||||||
|
||||||
if (_blocks.TryDequeue(out var block)) | ||||||
{ | ||||||
_metrics?.UpdateCurrentMemory(-block.Memory.Length); | ||||||
_metrics?.Rent(block.Memory.Length); | ||||||
Interlocked.Add(ref _currentMemory, -block.Memory.Length); | ||||||
|
||||||
// block successfully taken from the stack - return it | ||||||
return block; | ||||||
} | ||||||
|
||||||
_metrics?.IncrementTotalMemory(BlockSize); | ||||||
_metrics?.Rent(BlockSize); | ||||||
|
||||||
// We already counted this Rent call above, but since we're now allocating (need more blocks) | ||||||
// that means the pool is 'very' active and we probably shouldn't evict blocks, so we count again | ||||||
// to reduce the chance of eviction occurring this cycle. | ||||||
Interlocked.Increment(ref _rentCount); | ||||||
|
||||||
return new MemoryPoolBlock(this, BlockSize); | ||||||
} | ||||||
|
||||||
|
@@ -82,12 +134,94 @@ internal void Return(MemoryPoolBlock block) | |||||
block.IsLeased = false; | ||||||
#endif | ||||||
|
||||||
Interlocked.Increment(ref _returnCount); | ||||||
|
||||||
if (!_isDisposed) | ||||||
{ | ||||||
_metrics?.UpdateCurrentMemory(block.Memory.Length); | ||||||
Interlocked.Add(ref _currentMemory, block.Memory.Length); | ||||||
|
||||||
_blocks.Enqueue(block); | ||||||
} | ||||||
} | ||||||
|
||||||
public bool TryScheduleEviction(DateTimeOffset now) | ||||||
{ | ||||||
if (now >= _nextEviction) | ||||||
{ | ||||||
_nextEviction = now.Add(DefaultEvictionDelay); | ||||||
ThreadPool.UnsafeQueueUserWorkItem(this, preferLocal: false); | ||||||
return true; | ||||||
} | ||||||
|
||||||
return false; | ||||||
} | ||||||
|
||||||
void IThreadPoolWorkItem.Execute() | ||||||
{ | ||||||
try | ||||||
{ | ||||||
PerformEviction(); | ||||||
} | ||||||
catch (Exception ex) | ||||||
{ | ||||||
// Log the exception, but don't let it crash the thread pool | ||||||
_logger?.LogError(ex, "Error while performing eviction in PinnedBlockMemoryPool."); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think a critical log is appropriate considering something must have gone horribly wrong to get here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I chose error because we're still going to be running eviction code next iteration. Critical to me feels more like we stopped doing work now and in the future because something is horribly wrong. i.e. the accept loop in Kestrel: https://source.dot.net/#Microsoft.AspNetCore.Server.Kestrel.Core/Internal/ConnectionDispatcher.cs,74 |
||||||
} | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Examines the current usage and activity of the memory pool and evicts a calculated number of unused memory blocks. | ||||||
/// The eviction policy is adaptive: if the pool is underutilized or idle, more blocks are evicted; | ||||||
/// if activity is high, fewer or no blocks are evicted. | ||||||
/// Evicted blocks are removed from the pool and their memory is unrooted for garbage collection. | ||||||
/// </summary> | ||||||
internal void PerformEviction() | ||||||
BrennanConroy marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do other heartbeat activities log? I'm wondering if there should be some trace-level logging here. For example, if the memory pool decides to evict memory then write a log with details about why and how much. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kestrel does eviction of Http2Stream streams on the heartbeat. I don't think it's logged... |
||||||
{ | ||||||
var currentCount = (uint)_blocks.Count; | ||||||
var burstAmount = 0u; | ||||||
|
||||||
var rentCount = _rentCount; | ||||||
var returnCount = _returnCount; | ||||||
_rentCount = 0; | ||||||
_returnCount = 0; | ||||||
|
||||||
// If any activity | ||||||
if (rentCount + returnCount > 0) | ||||||
{ | ||||||
// Trending less traffic | ||||||
if (returnCount > rentCount) | ||||||
{ | ||||||
// Remove the lower of 1% of the current blocks and 20% of the difference between rented and returned | ||||||
burstAmount = Math.Min(currentCount / 100, (returnCount - rentCount) / 5); | ||||||
} | ||||||
// Traffic staying the same, try removing some blocks since we probably have excess | ||||||
else if (returnCount == rentCount) | ||||||
{ | ||||||
// Remove 1% of the current blocks (or at least 1) | ||||||
burstAmount = Math.Max(1, currentCount / 100); | ||||||
} | ||||||
// else trending more traffic so we don't want to evict anything | ||||||
} | ||||||
// If no activity | ||||||
else | ||||||
{ | ||||||
// Remove 5% of the current blocks (or at least 10) | ||||||
burstAmount = Math.Max(10, currentCount / 20); | ||||||
} | ||||||
|
||||||
// Remove from queue and let GC clean the memory up | ||||||
while (burstAmount > 0 && _blocks.TryDequeue(out var block)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it'd be interesting to emit some metric if this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leave a comment on #61594? That could happen any time we are performing eviction and new traffic starts coming in, or if the pool was low and the heuristics didn't limit themselves to the size of the pool. |
||||||
{ | ||||||
_metrics?.UpdateCurrentMemory(-block.Memory.Length); | ||||||
_metrics?.EvictBlock(block.Memory.Length); | ||||||
Interlocked.Add(ref _currentMemory, -block.Memory.Length); | ||||||
Interlocked.Add(ref _evictedMemory, block.Memory.Length); | ||||||
|
||||||
burstAmount--; | ||||||
} | ||||||
} | ||||||
|
||||||
protected override void Dispose(bool disposing) | ||||||
{ | ||||||
if (_isDisposed) | ||||||
|
@@ -97,8 +231,15 @@ protected override void Dispose(bool disposing) | |||||
|
||||||
lock (_disposeSync) | ||||||
{ | ||||||
if (_isDisposed) | ||||||
{ | ||||||
return; | ||||||
} | ||||||
|
||||||
_isDisposed = true; | ||||||
|
||||||
_onPoolDisposed?.Invoke(_onPoolDisposedState, this); | ||||||
BrennanConroy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
if (disposing) | ||||||
{ | ||||||
// Discard blocks in pool | ||||||
|
@@ -109,4 +250,7 @@ protected override void Dispose(bool disposing) | |||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
// Used for testing | ||||||
public int BlockCount() => _blocks.Count; | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Diagnostics.Metrics; | ||
|
||
#nullable enable | ||
|
||
namespace Microsoft.AspNetCore; | ||
|
||
internal sealed class PinnedBlockMemoryPoolMetrics | ||
{ | ||
public const string MeterName = "Microsoft.AspNetCore.MemoryPool"; | ||
|
||
private readonly Meter _meter; | ||
private readonly UpDownCounter<long> _currentMemory; | ||
private readonly Counter<long> _totalAllocatedMemory; | ||
private readonly Counter<long> _evictedMemory; | ||
private readonly Counter<long> _totalRented; | ||
|
||
public PinnedBlockMemoryPoolMetrics(IMeterFactory meterFactory) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want a metric for peak? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. total_allocated covers that? Oh, nvm. Yeah, peak might be interesting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think peak is just measuring the max of |
||
{ | ||
_meter = meterFactory.Create(MeterName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 However, I think a unit test to double check that is important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
_currentMemory = _meter.CreateUpDownCounter<long>( | ||
"aspnetcore.memorypool.current_memory", | ||
unit: "{bytes}", | ||
description: "Number of bytes that are currently pooled by the pool."); | ||
|
||
_totalAllocatedMemory = _meter.CreateCounter<long>( | ||
"aspnetcore.memorypool.total_allocated", | ||
unit: "{bytes}", | ||
description: "Total number of allocations made by the pool."); | ||
|
||
_evictedMemory = _meter.CreateCounter<long>( | ||
"aspnetcore.memorypool.evicted_memory", | ||
unit: "{bytes}", | ||
description: "Total number of bytes that have been evicted."); | ||
|
||
_totalRented = _meter.CreateCounter<long>( | ||
"aspnetcore.memorypool.total_rented", | ||
unit: "{bytes}", | ||
description: "Total number of rented bytes from the pool."); | ||
} | ||
|
||
public void UpdateCurrentMemory(int bytes) | ||
{ | ||
_currentMemory.Add(bytes); | ||
} | ||
|
||
public void IncrementTotalMemory(int bytes) | ||
{ | ||
_totalAllocatedMemory.Add(bytes); | ||
} | ||
|
||
public void EvictBlock(int bytes) | ||
{ | ||
_evictedMemory.Add(bytes); | ||
} | ||
|
||
public void Rent(int bytes) | ||
{ | ||
_totalRented.Add(bytes); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we at all concerned with breaking reflection-based code that was setting this previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. The couple teams we gave code to were told the private reflection wouldn't work in 10.0.