Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Improved Memoryblock tracking #998

Merged
merged 3 commits into from
Jul 28, 2016
Merged

Improved Memoryblock tracking #998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b8abf5
Improved Memoryblock tracking
benaadams Jul 20, 2016
aa385a1
Use Environment.NewLine and Environment.StackTrace
benaadams Jul 20, 2016
f42316a
Remove Environment.StackTrace as crashing tests
benaadams Jul 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Concurrent;
using System.Diagnostics;
using System.Runtime.CompilerServices;

namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure
{
Expand Down Expand Up @@ -64,16 +65,33 @@ public class MemoryPool : IDisposable
/// Called to take a block from the pool.
/// </summary>
/// <returns>The block that is reserved for the called. It must be passed to Return when it is no longer being used.</returns>
#if DEBUG
public MemoryPoolBlock Lease([CallerMemberName] string memberName = "",
[CallerFilePath] string sourceFilePath = "",
[CallerLineNumber] int sourceLineNumber = 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the switching Environment.StackTrace to this that made the tests faster?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another one of my super nits :) Put the first arg in its own line, and indent all args with 4 spaces only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Environment.StackTrace caused the large response test to crash, which then caused a block to finalize. On the one hand it crashed; on the other it finalized a block - so may still be a circumstance...

{
Debug.Assert(!_disposedValue, "Block being leased from disposed pool!");
#else
public MemoryPoolBlock Lease()
{
#endif
MemoryPoolBlock block;
if (_blocks.TryDequeue(out block))
{
// block successfully taken from the stack - return it
#if DEBUG
block.Leaser = memberName + ", " + sourceFilePath + ", " + sourceLineNumber;
block.IsLeased = true;
#endif
return block;
}
// no blocks available - grow the pool
return AllocateSlab();
block = AllocateSlab();
#if DEBUG
block.Leaser = memberName + ", " + sourceFilePath + ", " + sourceLineNumber;
block.IsLeased = true;
#endif
return block;
}

/// <summary>
Expand All @@ -100,6 +118,9 @@ private MemoryPoolBlock AllocateSlab()
basePtr,
this,
slab);
#if DEBUG
block.IsLeased = true;
#endif
Return(block);
}

Expand All @@ -123,19 +144,33 @@ private MemoryPoolBlock AllocateSlab()
/// <param name="block">The block to return. It must have been acquired by calling Lease on the same memory pool instance.</param>
public void Return(MemoryPoolBlock block)
{
#if DEBUG
Debug.Assert(block.Pool == this, "Returned block was not leased from this pool");
Debug.Assert(block.IsLeased, $"Block being returned to pool twice: {block.Leaser}{Environment.NewLine}");
block.IsLeased = false;
#endif

if (block.Slab != null && block.Slab.IsActive)
{
block.Reset();
_blocks.Enqueue(block);
}
else
{
GC.SuppressFinalize(block);
}
}

protected virtual void Dispose(bool disposing)
{
if (!_disposedValue)
{
_disposedValue = true;
#if DEBUG
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
#endif
if (disposing)
{
MemoryPoolSlab slab;
Expand All @@ -146,7 +181,9 @@ protected virtual void Dispose(bool disposing)
}
}

foreach (var block in _blocks)
// Discard blocks in pool
MemoryPoolBlock block;
while (_blocks.TryDequeue(out block))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change. Did you actually see this race in your tests?

{
GC.SuppressFinalize(block);
}
Expand All @@ -155,7 +192,6 @@ protected virtual void Dispose(bool disposing)

// N/A: set large fields to null.

_disposedValue = true;
}
}

Expand Down
10 changes: 8 additions & 2 deletions src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolBlock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,16 @@ unsafe protected MemoryPoolBlock(IntPtr dataArrayPtr)
/// </summary>
public MemoryPoolBlock Next;

#if DEBUG
public bool IsLeased { get; set; }
public string Leaser { get; set; }
#endif

~MemoryPoolBlock()
{
Debug.Assert(Slab == null || !Slab.IsActive, "Block being garbage collected instead of returned to pool");

#if DEBUG
Debug.Assert(Slab == null || !Slab.IsActive, $"{Environment.NewLine}{Environment.NewLine}*** Block being garbage collected instead of returned to pool: {Leaser} ***{Environment.NewLine}");
#endif
if (Slab != null && Slab.IsActive)
{
Pool.Return(new MemoryPoolBlock(DataArrayPtr)
Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolSlab.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ protected virtual void Dispose(bool disposing)
{
if (!_disposedValue)
{
_disposedValue = true;

if (disposing)
{
// N/A: dispose managed state (managed objects).
Expand All @@ -72,8 +74,6 @@ protected virtual void Dispose(bool disposing)

// set large fields to null.
Array = null;

_disposedValue = true;
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/Microsoft.AspNetCore.Server.Kestrel/Internal/KestrelEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ public void Dispose()
thread.Stop(TimeSpan.FromSeconds(2.5));
}
Threads.Clear();
#if DEBUG
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
#endif
}

public IDisposable CreateServer(ServerAddress address)
Expand Down
7 changes: 6 additions & 1 deletion test/Microsoft.AspNetCore.Server.KestrelTests/AsciiDecoding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ private void ExceptionThrownForZeroOrNonAscii(byte b)
var end = GetIterator(begin, byteRange.Length);

Assert.Throws<BadHttpRequestException>(() => begin.GetAsciiString(end));

pool.Return(mem);
}
}
}
Expand Down Expand Up @@ -150,7 +152,10 @@ private void LargeAllocationProducesCorrectResults()
{
var returnBlock = block;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to keep this as is and remove

             pool.Return(mem0);
             pool.Return(mem1);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably :)

block = block.Next;
pool.Return(returnBlock);
if (returnBlock != mem0 && returnBlock != mem1)
{
pool.Return(returnBlock);
}
}

pool.Return(mem0);
Expand Down
1 change: 1 addition & 0 deletions test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ public void SkipThrowsWhenSkippingMoreBytesThanAvailableInMultipleBlocks()
Assert.ThrowsAny<InvalidOperationException>(() => scan.Skip(8));

_pool.Return(block);
_pool.Return(nextBlock);
}

[Theory]
Expand Down