Skip to content
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.

Sign up for GitHub

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

Better BlobBuilder pooling #72383

Merged
merged 12 commits into from
Mar 12, 2024
Merged

Better BlobBuilder pooling #72383

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
03a61be
Better BlobBuilder pooling
jaredpar Mar 4, 2024
4351f58
more
jaredpar Mar 4, 2024
06b1763
more
jaredpar Mar 4, 2024
d2e8a20
pr feedback
jaredpar Mar 5, 2024
fdcf80e
pr feedback
jaredpar Mar 5, 2024
8f30134
nrt enable
jaredpar Mar 5, 2024
1101c64
pr feedback
jaredpar Mar 6, 2024
5e43343
pr feedback
jaredpar Mar 7, 2024
fc23dec
more
jaredpar Mar 8, 2024
814cd23
more
jaredpar Mar 8, 2024
e08abbd
more
jaredpar Mar 8, 2024
869a2eb
pr feedback
jaredpar Mar 12, 2024
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
19 changes: 19 additions & 0 deletions src/Compilers/Core/Portable/PEWriter/MetadataBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using Microsoft.Cci;

namespace Microsoft.CodeAnalysis;

internal static class MetadataBuilderExtensions
{
internal static BlobHandle GetOrAddBlobAndFree(this MetadataBuilder metadataBuilder, PooledBlobBuilder builder)
{
var handle = metadataBuilder.GetOrAddBlob(builder);
builder.Free();
return handle;
}
}
16 changes: 8 additions & 8 deletions src/Compilers/Core/Portable/PEWriter/MetadataWriter.PortablePdb.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ private BlobHandle SerializeSequencePoints(
return default(BlobHandle);
}

var writer = new BlobBuilder();
var writer = PooledBlobBuilder.GetInstance();

int previousNonHiddenStartLine = -1;
int previousNonHiddenStartColumn = -1;
Expand Down Expand Up @@ -699,7 +699,7 @@ private BlobHandle SerializeSequencePoints(
previousNonHiddenStartColumn = sequencePoints[i].StartColumn;
}

return _debugMetadataOpt.GetOrAddBlob(writer);
return _debugMetadataOpt.GetOrAddBlobAndFree(writer);
}

private static DebugSourceDocument TryGetSingleDocument(ImmutableArray<SequencePoint> sequencePoints)
Expand Down Expand Up @@ -809,38 +809,38 @@ private void SerializeEncMethodDebugInformation(IMethodBody methodBody, MethodDe

if (!encInfo.LocalSlots.IsDefaultOrEmpty)
{
var writer = new BlobBuilder();
var writer = PooledBlobBuilder.GetInstance();

encInfo.SerializeLocalSlots(writer);

_debugMetadataOpt.AddCustomDebugInformation(
parent: method,
kind: _debugMetadataOpt.GetOrAddGuid(PortableCustomDebugInfoKinds.EncLocalSlotMap),
value: _debugMetadataOpt.GetOrAddBlob(writer));
value: _debugMetadataOpt.GetOrAddBlobAndFree(writer));
}

if (!encInfo.Lambdas.IsDefaultOrEmpty)
{
var writer = new BlobBuilder();
var writer = PooledBlobBuilder.GetInstance();

encInfo.SerializeLambdaMap(writer);

_debugMetadataOpt.AddCustomDebugInformation(
parent: method,
kind: _debugMetadataOpt.GetOrAddGuid(PortableCustomDebugInfoKinds.EncLambdaAndClosureMap),
value: _debugMetadataOpt.GetOrAddBlob(writer));
value: _debugMetadataOpt.GetOrAddBlobAndFree(writer));
}

if (!encInfo.StateMachineStates.IsDefaultOrEmpty)
{
var writer = new BlobBuilder();
var writer = PooledBlobBuilder.GetInstance();

encInfo.SerializeStateMachineStates(writer);

_debugMetadataOpt.AddCustomDebugInformation(
parent: method,
kind: _debugMetadataOpt.GetOrAddGuid(PortableCustomDebugInfoKinds.EncStateMachineStateMap),
value: _debugMetadataOpt.GetOrAddBlob(writer));
value: _debugMetadataOpt.GetOrAddBlobAndFree(writer));
}
}

Expand Down
22 changes: 18 additions & 4 deletions src/Compilers/Core/Portable/PEWriter/PeWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ internal static bool WritePeToStream(

var ilBuilder = new BlobBuilder(32 * 1024);
var mappedFieldDataBuilder = new BlobBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't these BlobBuilders pooled as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

The ilBuilder instance is not pooled because it is significantly larger than the instances that we have pools for. I considered creating a separate pool for ilBuilder but it's complicated. I wanted to get past this change, dig deeper into the profiles and if this proves to be a significant source of allocations then I'll come back to it.

The mappedFieldDataBuilder wasn't shown to be a significant source of allocations hence I hadn't done the work yet to establish the ownership. Unfortunately unlike most other pooling decisions this isn't a simple "it's that type, use the pool" decision. Instead have to sit down and map out which BlobBuilder actually owns the instance.

var managedResourceBuilder = new BlobBuilder(1024);
var managedResourceBuilder = PooledBlobBuilder.GetInstance();
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

PooledBlobBuilder.GetInstance();

We usually prefer a pattern where, the one getting things from the pool is the one responsible for putting it back. It doesn't look like the pattern is used for this allocation #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

The pattern for BlobBuilder is unfortunately more complicated. Yes in most cases it's the case that the initial getter is the one who puts it back in the pool. That is the typical pattern for simple uses like building up signatures, sequence points, etc ...

For the more complicated cases like building up the PE file the BlobBuilder can be linked into a different BlobBuilder via LinkSuffix / LinkPrefix. At that point the BlobBuilder they're linked into becomes the owner and is responsible for freeing the instance. That is the case here. The managedResourceBuilder will be linked into the peBuilder instance and that is the responsibility for freeing.

I'm open to suggestions on how to best express this. I tried a few patterns for asserting that values were properly freed but @333fred rightfully pointed out that they were race conditions. Can try and comment better the ownership at the point of allocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm open to suggestions on how to best express this.

I suggest to try the following. If some API can take an ownership of a builder, the builder should be passed by reference. If ownership is taken, the ref parameter should be set to null. The caller always frees if the builder is not null.

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory I agree with this principal. Unfortunately, many of the API we are dealing with here are in SRM and are existing public API. Can't change them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Think I have an idea on how to clear this up a bit. Going to try and encapsulate this weird ownership problem for this method into a struct that is hopefully easier to reason about.


Blob mvidFixup, mvidStringFixup;
mdWriter.BuildMetadataAndIL(
Expand Down Expand Up @@ -155,7 +155,7 @@ internal static bool WritePeToStream(
// We need to calculate the PDB checksum, so we may as well use the calculated hash for PDB ID regardless of whether deterministic build is requested.
var portablePdbContentHash = default(ImmutableArray<byte>);

BlobBuilder portablePdbToEmbed = null;
PooledBlobBuilder portablePdbToEmbed = null;
if (mdWriter.EmitPortableDebugMetadata)
{
mdWriter.AddRemainingDebugDocuments(mdWriter.Module.DebugDocumentsBuilder.DebugDocuments);
Expand All @@ -167,7 +167,7 @@ internal static bool WritePeToStream(
new Func<IEnumerable<Blob>, BlobContentId>(content => BlobContentId.FromHash(portablePdbContentHash = CryptographicHashProvider.ComputeHash(context.Module.PdbChecksumAlgorithm, content))) :
null;

var portablePdbBlob = new BlobBuilder();
var portablePdbBlob = PooledBlobBuilder.GetInstance(zero: true);
var portablePdbBuilder = mdWriter.GetPortablePdbBuilder(metadataRootBuilder.Sizes.RowCounts, debugEntryPointHandle, portablePdbIdProvider);
pdbContentId = portablePdbBuilder.Serialize(portablePdbBlob);
portablePdbVersion = portablePdbBuilder.FormatVersion;
Expand All @@ -186,6 +186,7 @@ internal static bool WritePeToStream(
try
{
portablePdbBlob.WriteContentTo(portablePdbStream);
portablePdbBlob.Free();
}
catch (Exception e) when (!(e is OperationCanceledException))
{
Expand Down Expand Up @@ -221,6 +222,7 @@ internal static bool WritePeToStream(
if (portablePdbToEmbed != null)
{
debugDirectoryBuilder.AddEmbeddedPortablePdbEntry(portablePdbToEmbed, portablePdbVersion);
portablePdbToEmbed.Free();
}
}
else
Expand All @@ -231,6 +233,12 @@ internal static bool WritePeToStream(
var strongNameProvider = context.Module.CommonCompilation.Options.StrongNameProvider;
var corFlags = properties.CorFlags;

if (managedResourceBuilder.Count == 0)
Copy link
Member Author

@jaredpar jaredpar Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are essentially two cases here:

  1. The managedResourceBuilder has content in which case it will be linked, via LinkSuffix, to peBlob below. Thus freeing peBlob frees both.
  2. The managedResourceBuilder has no content thus LinkSuffix does nothing and is discarded. This means it is not returned to the pool.

After some thought decided the easiest approach here was to just discard managedResourceBuilder here if it's unused. It's optional below so this is not a functional change. Happy to consider other approaches.

The fact that LinkSuffix and LinkPrefix don't call Free on 0-length chunks feels like a bug but waiting for @tmat to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Went ahead and filed a bug dotnet/runtime#99266

{
managedResourceBuilder.Free();
managedResourceBuilder = null;
}

var peBuilder = new ExtendedPEBuilder(
peHeaderBuilder,
metadataRootBuilder,
Expand All @@ -245,7 +253,11 @@ internal static bool WritePeToStream(
peIdProvider,
metadataOnly && !context.IncludePrivateMembers);

var peBlob = new BlobBuilder();
// This needs to force the backing builder to zero due to the issue writing COFF
// headers. Can remove once this issue is fixed and we've moved to SRM with the
// fix
// https://github.com/dotnet/runtime/issues/99244
var peBlob = PooledBlobBuilder.GetInstance(zero: true);
var peContentId = peBuilder.Serialize(peBlob, out Blob mvidSectionFixup);

PatchModuleVersionIds(mvidFixup, mvidSectionFixup, mvidStringFixup, peContentId.Guid);
Expand All @@ -264,6 +276,8 @@ internal static bool WritePeToStream(
throw new PeWritingException(e);
}

peBlob.Free();

return true;
}

Expand Down
51 changes: 45 additions & 6 deletions src/Compilers/Core/Portable/PEWriter/PooledBlobBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,47 @@
using System.Reflection.Metadata;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis;
using System.Diagnostics;
jaredpar marked this conversation as resolved.
Show resolved Hide resolved

namespace Microsoft.Cci
{
internal sealed class PooledBlobBuilder : BlobBuilder, IDisposable
{
private const int PoolSize = 128;
private const int ChunkSize = 1024;
private const int PoolChunkSize = 1024;

private static readonly ObjectPool<PooledBlobBuilder> s_chunkPool = new ObjectPool<PooledBlobBuilder>(() => new PooledBlobBuilder(ChunkSize), PoolSize);
private static readonly ObjectPool<PooledBlobBuilder> s_chunkPool = new ObjectPool<PooledBlobBuilder>(() => new PooledBlobBuilder(PoolChunkSize), PoolSize);

private PooledBlobBuilder(int size)
: base(size)
{
}

public static PooledBlobBuilder GetInstance()
/// <summary>
/// Get a new instance of the <see cref="BlobBuilder"/> that has <see cref="BlobBuilder.ChunkCapacity"/> of
/// at least <see cref="PoolChunkSize"/>
/// </summary>
/// <param name="zero">When true force zero out the backing buffer</param>
/// <remarks>
/// The <paramref name="zero"/> can be removed when moving to SRM 9.0 if it contains the bug fix for
/// <see cref="BlobBuilder.ReserveBytes(int)"/>
///
/// https://github.com/dotnet/runtime/issues/99244
/// </remarks>
public static PooledBlobBuilder GetInstance(bool zero = false)
{
return s_chunkPool.Allocate();
var builder = s_chunkPool.Allocate();
if (zero)
{
builder.WriteBytes(0, builder.ChunkCapacity);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

builder.ChunkCapacity

Is this total memory size behind the builder? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is the size of the underlying byte[] in the BlobBuilder instance.

builder.Clear();
}
return builder;
}

protected override BlobBuilder AllocateChunk(int minimalSize)
{
if (minimalSize <= ChunkSize)
if (minimalSize <= PoolChunkSize)
{
return s_chunkPool.Allocate();
}
Expand All @@ -38,7 +56,28 @@ protected override BlobBuilder AllocateChunk(int minimalSize)

protected override void FreeChunk()
{
s_chunkPool.Free(this);
if (ChunkCapacity != PoolChunkSize)
{
// The invariant of this builder is that it produces BlobBuilder instances that have a
// ChunkCapacity of at least 1024. Essentially inside AllocateChuck the pool must be able
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
// to mindlessly allocate a BlobBuilder where ChunkCapacity is at least 1024.
//
// To maintain this the code must verify that the returned BlobBuilder instances have
// a backing array of the appropriate size. This array can shrink in practice through code
// like the following:
//
// var builder = PooledBlobBuilder.GetInstance();
// builder.LinkSuffix(new BlobBuilder(256));
// builder.Free(); // calls FreeChunk where ChunkCapacity is 256
//
// This shouldn't happen much in practice due to convention of how builders are used but
// it is a legal use of the APIs.
s_chunkPool.ForgetTrackedObject(this);
}
else
{
s_chunkPool.Free(this);
}
}

public new void Free()
Expand Down