Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

#1991 - ObjectPool approach #3702

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions src/System.IO.Compression/System.IO.Compression.sln
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,7 @@ Global
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
EndGlobalSection
GlobalSection(CodealikeProperties) = postSolution
SolutionGuid = a7e376ea-0587-47fe-88c2-fd75be4b20bb
EndGlobalSection
EndGlobal
1 change: 1 addition & 0 deletions src/System.IO.Compression/src/System.IO.Compression.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
<Compile Include="System\IO\Compression\InflaterState.cs" />
<Compile Include="System\IO\Compression\InflaterZlib.cs" />
<Compile Include="System\IO\Compression\InputBuffer.cs" />
<Compile Include="System\IO\Compression\Internal\ObjectPool`1.cs" />
<Compile Include="System\IO\Compression\Match.cs" />
<Compile Include="System\IO\Compression\MatchState.cs" />
<Compile Include="System\IO\Compression\OutputBuffer.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,22 @@
using System.Threading;
using System.Threading.Tasks;
using System.Diagnostics.Contracts;
using System.IO.Compression.Internal;

namespace System.IO.Compression
{
public partial class DeflateStream : Stream
{
private static readonly ObjectPool<byte[]> s_byteBufferPool = new ObjectPool<byte[]>(() => new byte[DefaultBufferSize], 32);

internal const int DefaultBufferSize = 8192;

private Stream _stream;
private CompressionMode _mode;
private bool _leaveOpen;
private IInflater _inflater;
private IDeflater _deflater;
private byte[] _buffer;
private readonly byte[] _buffer;

private int _asyncOperations;

Expand Down Expand Up @@ -50,8 +53,8 @@ internal DeflateStream(Stream stream, bool leaveOpen, IFileFormatReader reader)
_inflater = CreateInflater(reader);
_stream = stream;
_mode = CompressionMode.Decompress;
_leaveOpen = leaveOpen;
_buffer = new byte[DefaultBufferSize];
_leaveOpen = leaveOpen;
_buffer = s_byteBufferPool.Allocate();
}


Expand Down Expand Up @@ -85,7 +88,7 @@ public DeflateStream(Stream stream, CompressionMode mode, bool leaveOpen)
_stream = stream;
_mode = mode;
_leaveOpen = leaveOpen;
_buffer = new byte[DefaultBufferSize];
_buffer = s_byteBufferPool.Allocate();
}

// Implies mode = Compress
Expand Down Expand Up @@ -115,7 +118,7 @@ public DeflateStream(Stream stream, CompressionLevel compressionLevel, bool leav

_deflater = CreateDeflater(compressionLevel);

_buffer = new byte[DefaultBufferSize];
_buffer = s_byteBufferPool.Allocate();
}

private static IDeflater CreateDeflater(CompressionLevel? compressionLevel)
Expand Down Expand Up @@ -611,6 +614,8 @@ protected override void Dispose(bool disposing)
base.Dispose(disposing);
}
} // finally

s_byteBufferPool.Free(_buffer);
Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar with the deflater codebase, but quite familiar with this pool. One thing to watch out when using ObjectPool, especially if it is static, is to not return back large objects.
If _buffer may expand, make sure you are not returning really big ones.

Copy link
Author

Choose a reason for hiding this comment

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

It does not, they are all the same size. 32Kb and 8Kb (all below the LOH threshold).

Copy link
Member

Choose a reason for hiding this comment

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

Pool limits the number of retained objects, but you also need to ensure a limit for the size of stored objects. Otherwise the cache is unbounded - also known as memory leak.

Copy link
Author

Choose a reason for hiding this comment

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

I selected the size to ensure that a max of 1MB of memory is consumed, as arrays are all of the same size (and will never change, _buffer is readonly just in case) and we can know that only 32 are going to be retained.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good then.

} // finally
} // Dispose

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,10 @@ private bool DecodeDynamicBlockHeader()
return true;
}

public void Dispose() { }
public void Dispose()
{
_output.Dispose();
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Threading;

namespace System.IO.Compression.Internal
{
/// <summary>
/// Generic implementation of object pooling pattern with predefined pool size limit. The main
/// purpose is that limited number of frequently used objects can be kept in the pool for
/// further recycling.
///
/// Notes:
/// 1) it is not the goal to keep all returned objects. Pool is not meant for storage. If there
/// is no space in the pool, extra returned objects will be dropped.
///
/// 2) it is implied that if object was obtained from a pool, the caller will return it back in
/// a relatively short time. Keeping checked out objects for long durations is ok, but
/// reduces usefulness of pooling. Just new up your own.
///
/// Not returning objects to the pool in not detrimental to the pool's work, but is a bad practice.
/// Rationale:
/// If there is no intent for reusing the object, do not use pool - just use "new".
/// </summary>
internal sealed class ObjectPool<T> where T : class
{
private struct Element
{
internal T Value;
}

// storage for the pool objects.
private readonly Element[] _items;

// factory is stored for the lifetime of the pool. We will call this only when pool needs to
// expand. compared to "new T()", Func gives more flexibility to implementers and faster
// than "new T()".
private readonly Func<T> _factory;


internal ObjectPool(Func<T> factory)
: this(factory, Environment.ProcessorCount * 2)
{ }

internal ObjectPool(Func<T> factory, int size)
{
_factory = factory;
_items = new Element[size];
}

private T CreateInstance()
{
var inst = _factory();
return inst;
}

/// <summary>
/// Produces an instance.
/// </summary>
/// <remarks>
/// Search strategy is a simple linear probing which is chosen for it cache-friendliness.
/// Note that Free will try to store recycled objects close to the start thus statistically
/// reducing how far we will typically search.
/// </remarks>
internal T Allocate()
{
var items = _items;
Copy link
Contributor

Choose a reason for hiding this comment

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

❗ coding style violation: rule # 10

Copy link
Author

Choose a reason for hiding this comment

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

@Maxwe11 I actually took this verbatim from: https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ObjectPool%601.cs

I was signing the CLA and then opening a new ticket to make ObjectPool available throught all CoreFX to avoid redundant code-copying.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case I'd rather apply git mv on this file and move it to src/Common folder for reuse 😄

Copy link

Choose a reason for hiding this comment

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

I was signing the CLA and then opening a new ticket to make...

Apparently the first part didn't went through. 😄

Copy link
Author

Choose a reason for hiding this comment

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

No it didnt. Sent an email to the foundation requesting support. Not even using the Access documents option worked.

Copy link
Author

Choose a reason for hiding this comment

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

@stephentoub I didn't get any response from either the ticket I opened to DocuSign and the email I sent to the DotNet Foundation that is specified in the email. Do you know who I could contact to be able to sign the CLA? Because it is not working, tried in Chrome and Edge, no luck.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@redknightlois, I spoke with @martinwoodward at the foundation, and he's concerned because he didn't see any email from you nor any issues with signing. He asked that you resend your email and include him directly: martin@dotnetfoundation.org. Sorry for the hassle, and thanks for persisting.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Forwarded the original email to Martin.

T inst;

for (int i = 0; i < items.Length; i++)
{
// Note that the read is optimistically not synchronized. That is intentional.
// We will interlock only when we have a candidate. in a worst case we may miss some
// recently returned objects. Not a big deal.
inst = items[i].Value;
if (inst != null)
{
if (inst == Interlocked.CompareExchange(ref items[i].Value, null, inst))
{
goto gotInstance;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO It would be better to use boolean flag and eliminate usage of goto

Copy link
Member

Choose a reason for hiding this comment

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

We are competing with new/GC here on the basis that we know the scenario and can do better.
To be profitable this piece needs to be fast and that often requires some code patterns you would not otherwise use. I'd ignore goto here.

BTW, Roslyn has a slightly newer version that has special treatment for situations when a single instance is sufficient in most cases, which is relatively common. It basically adds a singleton stash in addition to an array.

https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/SharedCollections/ObjectPool%601.cs

Copy link
Author

Choose a reason for hiding this comment

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

I am fairly acquainted with the Roslyn ObjectPool, it is the general consensus to use that one instead on CoreFX? It feels a bit too broad a change for me to do on my first PR :)

Copy link
Member

Choose a reason for hiding this comment

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

All the leak tracking goo is unnecessary. You can get same data from any profiler if needed. The only interesting difference there is that additional single item cache.
Yes, that can be added later.

}
}
}

inst = CreateInstance();
gotInstance:

return inst;
}

/// <summary>
/// Returns objects to the pool.
/// </summary>
/// <remarks>
/// Search strategy is a simple linear probing which is chosen for it cache-friendliness.
/// Note that Free will try to store recycled objects close to the start thus statistically
/// reducing how far we will typically search in Allocate.
/// </remarks>
internal void Free(T obj)
{
var items = _items;
for (int i = 0; i < items.Length; i++)
{
if (items[i].Value == null)
{
// Intentionally not using interlocked here.
// In a worst case scenario two objects may be stored into same slot.
// It is very unlikely to happen and will only mean that one of the objects will get collected.
items[i].Value = obj;
break;
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Diagnostics;
using System.Globalization;
using System.IO.Compression.Internal;

namespace System.IO.Compression
{
Expand All @@ -13,15 +14,22 @@ namespace System.IO.Compression
// we need to look back in the output window and copy bytes from there.
// We use a byte array of WindowSize circularly.
//
internal class OutputWindow
internal class OutputWindow : IDisposable
{
private static readonly ObjectPool<byte[]> s_byteBufferPool = new ObjectPool<byte[]>(() => new byte[WindowSize], 16);

private const int WindowSize = 32768;
private const int WindowMask = 32767;

private byte[] _window = new byte[WindowSize]; //The window is 2^15 bytes
private readonly byte[] _window; //The window is 2^15 bytes
private int _end; // this is the position to where we should write next byte
private int _bytesUsed; // The number of bytes in the output window which is not consumed.

public OutputWindow()
{
_window = s_byteBufferPool.Allocate();
}

// Add a byte to output window
public void Write(byte b)
{
Expand Down Expand Up @@ -150,5 +158,31 @@ public int CopyTo(byte[] output, int offset, int length)
Debug.Assert(_bytesUsed >= 0, "check this function and find why we copied more bytes than we have");
return copied;
}

#region IDisposable Support

private bool disposedValue = false; // To detect redundant calls

protected virtual void Dispose(bool disposing)
{
if (!disposedValue)
{
if (disposing)
{
s_byteBufferPool.Free(_window);
}

disposedValue = true;
}
}

// This code added to correctly implement the disposable pattern.
public void Dispose()
{
// Do not change this code. Put cleanup code in Dispose(bool disposing) above.
Dispose(true);
}

#endregion
}
}