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 all 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
4 changes: 1 addition & 3 deletions src/Compilers/Core/Portable/Compilation/Compilation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3351,8 +3351,6 @@ internal static bool SerializePeToStream(
RSAParameters? privateKeyOpt,
CancellationToken cancellationToken)
{
bool emitSecondaryAssembly = getMetadataPeStreamOpt != null;

bool includePrivateMembersOnPrimaryOutput = metadataOnly ? includePrivateMembers : true;
bool deterministicPrimaryOutput = (metadataOnly && !includePrivateMembers) || isDeterministic;
if (!Cci.PeWriter.WritePeToStream(
Expand All @@ -3372,7 +3370,7 @@ internal static bool SerializePeToStream(
}

// produce the secondary output (ref assembly) if needed
if (emitSecondaryAssembly)
if (getMetadataPeStreamOpt 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.

This allowed me to avoid a nullable suppression on getMetadataPeStreamOpt below.

{
Debug.Assert(!metadataOnly);
Debug.Assert(!includePrivateMembers);
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Emit/DebugDocumentsBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ internal void AddDebugDocument(Cci.DebugSourceDocument document)
internal IReadOnlyDictionary<string, Cci.DebugSourceDocument> DebugDocuments
=> _debugDocuments;

internal Cci.DebugSourceDocument? TryGetDebugDocument(string path, string basePath)
internal Cci.DebugSourceDocument? TryGetDebugDocument(string path, string? basePath)
{
return TryGetDebugDocumentForNormalizedPath(NormalizeDebugDocumentPath(path, basePath));
}
Expand Down
10 changes: 5 additions & 5 deletions src/Compilers/Core/Portable/PEWriter/ExtendedPEBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ public ExtendedPEBuilder(
PEHeaderBuilder header,
MetadataRootBuilder metadataRootBuilder,
BlobBuilder ilStream,
BlobBuilder mappedFieldData,
BlobBuilder managedResources,
ResourceSectionBuilder nativeResources,
DebugDirectoryBuilder debugDirectoryBuilder,
BlobBuilder? mappedFieldData,
BlobBuilder? managedResources,
ResourceSectionBuilder? nativeResources,
DebugDirectoryBuilder? debugDirectoryBuilder,
int strongNameSignatureSize,
MethodDefinitionHandle entryPoint,
CorFlags flags,
Func<IEnumerable<Blob>, BlobContentId> deterministicIdProvider,
Func<IEnumerable<Blob>, BlobContentId>? deterministicIdProvider,
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes all line up with the declarations on the base type ctor.

bool withMvidSection)
: base(header, metadataRootBuilder, ilStream, mappedFieldData, managedResources, nativeResources,
debugDirectoryBuilder, strongNameSignatureSize, entryPoint, flags, deterministicIdProvider)
Expand Down
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
108 changes: 63 additions & 45 deletions src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,27 +899,6 @@ private EntityHandle GetExportedTypeImplementation(INamespaceTypeReference names
: GetAssemblyReferenceHandle(aref);
}

private static uint GetManagedResourceOffset(ManagedResource resource, BlobBuilder resourceWriter)
{
if (resource.ExternalFile != null)
{
return resource.Offset;
}

int result = resourceWriter.Count;
resource.WriteData(resourceWriter);
return (uint)result;
}

private static uint GetManagedResourceOffset(BlobBuilder resource, BlobBuilder resourceWriter)
{
int result = resourceWriter.Count;
resourceWriter.WriteInt32(resource.Count);
resource.WriteContentTo(resourceWriter);
resourceWriter.Align(8);
return (uint)result;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These methods are only used in PopulateManifestResourceTableRows. Given the tight relationship that has to the impl details of these methods now I thought it better to make them local functions instead.


public static string GetMetadataName(INamedTypeReference namedType, int generation)
{
string nameWithGeneration = (generation == 0) ? namedType.Name : namedType.Name + "#" + generation;
Expand Down Expand Up @@ -1696,7 +1675,8 @@ internal EntityHandle GetDefinitionHandle(IDefinition definition)
};
}

public void WriteMetadataAndIL(PdbWriter nativePdbWriterOpt, Stream metadataStream, Stream ilStream, Stream portablePdbStreamOpt, out MetadataSizes metadataSizes)
#nullable enable
public void WriteMetadataAndIL(PdbWriter? nativePdbWriterOpt, Stream metadataStream, Stream ilStream, Stream? portablePdbStreamOpt, out MetadataSizes metadataSizes)
{
Debug.Assert(nativePdbWriterOpt == null ^ portablePdbStreamOpt == null);

Expand All @@ -1705,8 +1685,8 @@ public void WriteMetadataAndIL(PdbWriter nativePdbWriterOpt, Stream metadataStre
// TODO: we can precalculate the exact size of IL stream
var ilBuilder = new BlobBuilder(1024);
var metadataBuilder = new BlobBuilder(4 * 1024);
var mappedFieldDataBuilder = new BlobBuilder(0);
var managedResourceDataBuilder = new BlobBuilder(0);
PooledBlobBuilder? mappedFieldDataBuilder = null;
PooledBlobBuilder? managedResourceDataBuilder = null;

// Add 4B of padding to the start of the separated IL stream,
// so that method RVAs, which are offsets to this stream, are never 0.
Expand All @@ -1721,8 +1701,8 @@ public void WriteMetadataAndIL(PdbWriter nativePdbWriterOpt, Stream metadataStre
BuildMetadataAndIL(
nativePdbWriterOpt,
ilBuilder,
mappedFieldDataBuilder,
managedResourceDataBuilder,
out mappedFieldDataBuilder,
out managedResourceDataBuilder,
out Blob mvidFixup,
out Blob mvidStringFixup);

Expand All @@ -1731,16 +1711,16 @@ public void WriteMetadataAndIL(PdbWriter nativePdbWriterOpt, Stream metadataStre
Debug.Assert(typeSystemRowCounts[(int)TableIndex.EncMap] == 0);
PopulateEncTables(typeSystemRowCounts);

Debug.Assert(mappedFieldDataBuilder.Count == 0);
Debug.Assert(managedResourceDataBuilder.Count == 0);
Debug.Assert(mappedFieldDataBuilder == null);
Debug.Assert(managedResourceDataBuilder == null);
Debug.Assert(mvidFixup.IsDefault);
Debug.Assert(mvidStringFixup.IsDefault);

// TODO: Update SRM to not sort Custom Attribute table when emitting EnC delta
// https://github.com/dotnet/roslyn/issues/70389
if (!IsFullMetadata)
{
metadata.GetType().GetField("_customAttributeTableNeedsSorting", BindingFlags.Instance | BindingFlags.NonPublic).SetValue(metadata, false);
metadata.GetType().GetField("_customAttributeTableNeedsSorting", BindingFlags.Instance | BindingFlags.NonPublic)!.SetValue(metadata, false);
}

// TODO (https://github.com/dotnet/roslyn/issues/3905):
Expand Down Expand Up @@ -1783,10 +1763,10 @@ public void WriteMetadataAndIL(PdbWriter nativePdbWriterOpt, Stream metadataStre
}

public void BuildMetadataAndIL(
PdbWriter nativePdbWriterOpt,
PdbWriter? nativePdbWriterOpt,
BlobBuilder ilBuilder,
BlobBuilder mappedFieldDataBuilder,
BlobBuilder managedResourceDataBuilder,
out PooledBlobBuilder? mappedFieldDataBuilder,
out PooledBlobBuilder? managedResourceDataBuilder,
out Blob mvidFixup,
out Blob mvidStringFixup)
{
Expand Down Expand Up @@ -1849,15 +1829,17 @@ public void BuildMetadataAndIL(

ReportReferencesToAddedSymbols();

BlobBuilder dynamicAnalysisDataOpt = null;
PooledBlobBuilder? dynamicAnalysisData = null;
if (_dynamicAnalysisDataWriterOpt != null)
{
dynamicAnalysisDataOpt = new BlobBuilder();
_dynamicAnalysisDataWriterOpt.SerializeMetadataTables(dynamicAnalysisDataOpt);
dynamicAnalysisData = PooledBlobBuilder.GetInstance();
_dynamicAnalysisDataWriterOpt.SerializeMetadataTables(dynamicAnalysisData);
}

PopulateTypeSystemTables(methodBodyOffsets, mappedFieldDataBuilder, managedResourceDataBuilder, dynamicAnalysisDataOpt, out mvidFixup);
PopulateTypeSystemTables(methodBodyOffsets, out mappedFieldDataBuilder, out managedResourceDataBuilder, dynamicAnalysisData, out mvidFixup);
dynamicAnalysisData?.Free();
}
#nullable disable

public virtual void PopulateEncTables(ImmutableArray<int> typeSystemRowCounts)
{
Expand Down Expand Up @@ -1916,7 +1898,8 @@ private ImmutableArray<IGenericParameter> GetSortedGenericParameters()
}).ToImmutableArray();
}

private void PopulateTypeSystemTables(int[] methodBodyOffsets, BlobBuilder mappedFieldDataWriter, BlobBuilder resourceWriter, BlobBuilder dynamicAnalysisDataOpt, out Blob mvidFixup)
#nullable enable
private void PopulateTypeSystemTables(int[] methodBodyOffsets, out PooledBlobBuilder? mappedFieldDataWriter, out PooledBlobBuilder? resourceWriter, BlobBuilder? dynamicAnalysisData, out Blob mvidFixup)
{
var sortedGenericParameters = GetSortedGenericParameters();

Expand All @@ -1930,13 +1913,13 @@ private void PopulateTypeSystemTables(int[] methodBodyOffsets, BlobBuilder mappe
this.PopulateExportedTypeTableRows();
this.PopulateFieldLayoutTableRows();
this.PopulateFieldMarshalTableRows();
this.PopulateFieldRvaTableRows(mappedFieldDataWriter);
this.PopulateFieldRvaTableRows(out mappedFieldDataWriter);
this.PopulateFieldTableRows();
this.PopulateFileTableRows();
this.PopulateGenericParameters(sortedGenericParameters);
this.PopulateImplMapTableRows();
this.PopulateInterfaceImplTableRows();
this.PopulateManifestResourceTableRows(resourceWriter, dynamicAnalysisDataOpt);
this.PopulateManifestResourceTableRows(out resourceWriter, dynamicAnalysisData);
this.PopulateMemberRefTableRows();
this.PopulateMethodImplTableRows();
this.PopulateMethodTableRows(methodBodyOffsets);
Expand All @@ -1956,6 +1939,7 @@ private void PopulateTypeSystemTables(int[] methodBodyOffsets, BlobBuilder mappe
// This table is populated after the others because it depends on the order of the entries of the generic parameter table.
this.PopulateCustomAttributeTableRows(sortedGenericParameters);
}
#nullable disable

private void PopulateAssemblyRefTableRows()
{
Expand Down Expand Up @@ -2336,15 +2320,20 @@ private void PopulateFieldMarshalTableRows()
}
}

private void PopulateFieldRvaTableRows(BlobBuilder mappedFieldDataWriter)
#nullable enable
private void PopulateFieldRvaTableRows(out PooledBlobBuilder? mappedFieldDataWriter)
{
mappedFieldDataWriter = null;

foreach (IFieldDefinition fieldDef in this.GetFieldDefs())
{
if (fieldDef.MappedData.IsDefault)
{
continue;
}

mappedFieldDataWriter ??= PooledBlobBuilder.GetInstance();

// The compiler always aligns each RVA data field to an 8-byte boundary; this accomodates the alignment
// needs for all primitive types, regardless of which type is actually being used, at the expense of
// potentially wasting up to 7 bytes per field if the alignment needs are less. In the future, this
Expand All @@ -2362,6 +2351,7 @@ private void PopulateFieldRvaTableRows(BlobBuilder mappedFieldDataWriter)
offset: offset);
}
}
#nullable disable

private void PopulateFieldTableRows()
{
Expand Down Expand Up @@ -2417,6 +2407,7 @@ private void PopulateConstantTableRows()
continue;
}

Debug.Assert(propDef.DefaultValue != null);
metadata.AddConstant(
parent: GetPropertyDefIndex(propDef),
value: propDef.DefaultValue.Value);
Expand Down Expand Up @@ -2506,15 +2497,19 @@ private void PopulateInterfaceImplTableRows()
}
}

private void PopulateManifestResourceTableRows(BlobBuilder resourceDataWriter, BlobBuilder dynamicAnalysisDataOpt)
#nullable enable
private void PopulateManifestResourceTableRows(out PooledBlobBuilder? resourceDataWriter, BlobBuilder? dynamicAnalysisData)
{
if (dynamicAnalysisDataOpt != null)
resourceDataWriter = null;

if (dynamicAnalysisData != null)
{
resourceDataWriter = PooledBlobBuilder.GetInstance();
metadata.AddManifestResource(
attributes: ManifestResourceAttributes.Private,
name: metadata.GetOrAddString("<DynamicAnalysisData>"),
implementation: default(EntityHandle),
offset: GetManagedResourceOffset(dynamicAnalysisDataOpt, resourceDataWriter)
offset: writeBuilderResourceAndGetOffset(dynamicAnalysisData, resourceDataWriter)
);
}

Expand All @@ -2536,12 +2531,35 @@ private void PopulateManifestResourceTableRows(BlobBuilder resourceDataWriter, B
attributes: resource.IsPublic ? ManifestResourceAttributes.Public : ManifestResourceAttributes.Private,
name: GetStringHandleForNameAndCheckLength(resource.Name),
implementation: implementation,
offset: GetManagedResourceOffset(resource, resourceDataWriter));
offset: writeManagedResourceAndGetOffset(resource, ref resourceDataWriter));
}

// the stream should be aligned:
Debug.Assert((resourceDataWriter.Count % ManagedPEBuilder.ManagedResourcesDataAlignment) == 0);
Debug.Assert(resourceDataWriter == null || (resourceDataWriter.Count % ManagedPEBuilder.ManagedResourcesDataAlignment) == 0);

static uint writeManagedResourceAndGetOffset(ManagedResource resource, ref PooledBlobBuilder? resourceWriter)
{
if (resource.ExternalFile != null)
{
return resource.Offset;
}

resourceWriter ??= PooledBlobBuilder.GetInstance();
int result = resourceWriter.Count;
resource.WriteData(resourceWriter);
return (uint)result;
}

static uint writeBuilderResourceAndGetOffset(BlobBuilder resource, BlobBuilder resourceWriter)
{
int result = resourceWriter.Count;
resourceWriter.WriteInt32(resource.Count);
resource.WriteContentTo(resourceWriter);
resourceWriter.Align(8);
return (uint)result;
}
}
#nullable disable

private void PopulateMemberRefTableRows()
{
Expand Down
Loading
Loading