From dd87324ec891e3008a45809d6b4fdeadf5fe1d56 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Wed, 20 Mar 2024 21:56:57 +0100 Subject: [PATCH 1/4] BlobBuilder.LinkSuffix/LinkPrefix need to Free zero length chunks --- .../System/Reflection/Metadata/BlobBuilder.cs | 11 ++++++++++ .../tests/Metadata/BlobTests.cs | 21 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs index 093f60cf7197c..b406e4332bdf5 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics; using System.IO; @@ -32,6 +33,7 @@ public partial class BlobBuilder // This structure optimizes for append write operations and sequential enumeration from the start of the chain. // Data can only be written to the head node. Other nodes are "frozen". private BlobBuilder _nextOrPrevious; + private List _emptyChildBlobs; private BlobBuilder FirstChunk => _nextOrPrevious._nextOrPrevious; // The sum of lengths of all preceding chunks (not including the current chunk), @@ -61,6 +63,7 @@ public BlobBuilder(int capacity = DefaultChunkSize) _nextOrPrevious = this; _buffer = new byte[Math.Max(MinChunkSize, capacity)]; + _emptyChildBlobs = new(); } protected virtual BlobBuilder AllocateChunk(int minimalSize) @@ -102,6 +105,12 @@ public void Clear() } } + foreach (BlobBuilder chunk in _emptyChildBlobs) + { + chunk.ClearChunk(); + chunk.FreeChunk(); + } + ClearChunk(); } @@ -396,6 +405,7 @@ public void LinkPrefix(BlobBuilder prefix) // avoid chaining empty chunks: if (prefix.Count == 0) { + _emptyChildBlobs.Add(prefix); return; } @@ -456,6 +466,7 @@ public void LinkSuffix(BlobBuilder suffix) // avoid chaining empty chunks: if (suffix.Count == 0) { + _emptyChildBlobs.Add(suffix); return; } diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs index 6c05f36d046c9..ca41072b9d99b 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs @@ -1039,6 +1039,27 @@ public void Pooled() builder2.Free(); } + [Fact] + public void FreeNonEmptySuffixAndPrefixBlobAndEmptyOnes() + { + var b1 = PooledBlobBuilder.GetInstance(); + var b2 = PooledBlobBuilder.GetInstance(); + var b3 = PooledBlobBuilder.GetInstance(); + var b4 = PooledBlobBuilder.GetInstance(); + var b5 = PooledBlobBuilder.GetInstance(); + + b1.WriteBytes(1, 1); + b2.WriteBytes(1, 1); + b3.WriteBytes(1, 1); + + b1.LinkSuffix(b2); + b1.LinkPrefix(b3); + b1.LinkSuffix(b4); + b1.LinkPrefix(b5); + + b1.Free(); + } + private class ProperStreamRead_TestStream : TestStreamBase { private readonly byte[] _sourceArray; From 72eb0c94a1159976561519e0a6b900694a9a0d77 Mon Sep 17 00:00:00 2001 From: Badre BSAILA <54767641+pedrobsaila@users.noreply.github.com> Date: Thu, 21 Mar 2024 11:56:30 +0100 Subject: [PATCH 2/4] free child blob list --- .../src/System/Reflection/Metadata/BlobBuilder.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs index b406e4332bdf5..8774e1fede3b7 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs @@ -105,10 +105,16 @@ public void Clear() } } - foreach (BlobBuilder chunk in _emptyChildBlobs) + if (_emptyChildBlobs.Count > 0) { - chunk.ClearChunk(); - chunk.FreeChunk(); + foreach (BlobBuilder chunk in _emptyChildBlobs) + { + chunk.ClearChunk(); + chunk.FreeChunk(); + } + + _emptyChildBlobs.Clear(); + _emptyChildBlobs.TrimExcess(); } ClearChunk(); From f4e0a9aab1f7ff3f3070a32b3c40dbcfebd1120f Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Sat, 30 Mar 2024 00:42:13 +0100 Subject: [PATCH 3/4] fix remarks 1 --- .../System/Reflection/Metadata/BlobBuilder.cs | 28 ++++++------------- .../tests/Metadata/BlobTests.cs | 21 -------------- 2 files changed, 9 insertions(+), 40 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs index 8774e1fede3b7..f89f76e723ca7 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics; using System.IO; @@ -33,7 +32,6 @@ public partial class BlobBuilder // This structure optimizes for append write operations and sequential enumeration from the start of the chain. // Data can only be written to the head node. Other nodes are "frozen". private BlobBuilder _nextOrPrevious; - private List _emptyChildBlobs; private BlobBuilder FirstChunk => _nextOrPrevious._nextOrPrevious; // The sum of lengths of all preceding chunks (not including the current chunk), @@ -63,7 +61,6 @@ public BlobBuilder(int capacity = DefaultChunkSize) _nextOrPrevious = this; _buffer = new byte[Math.Max(MinChunkSize, capacity)]; - _emptyChildBlobs = new(); } protected virtual BlobBuilder AllocateChunk(int minimalSize) @@ -100,23 +97,10 @@ public void Clear() { if (chunk != this) { - chunk.ClearChunk(); - chunk.FreeChunk(); + chunk.ClearAndFreeChunk(); } } - if (_emptyChildBlobs.Count > 0) - { - foreach (BlobBuilder chunk in _emptyChildBlobs) - { - chunk.ClearChunk(); - chunk.FreeChunk(); - } - - _emptyChildBlobs.Clear(); - _emptyChildBlobs.TrimExcess(); - } - ClearChunk(); } @@ -411,7 +395,7 @@ public void LinkPrefix(BlobBuilder prefix) // avoid chaining empty chunks: if (prefix.Count == 0) { - _emptyChildBlobs.Add(prefix); + prefix.ClearAndFreeChunk(); return; } @@ -472,7 +456,7 @@ public void LinkSuffix(BlobBuilder suffix) // avoid chaining empty chunks: if (suffix.Count == 0) { - _emptyChildBlobs.Add(suffix); + suffix.ClearAndFreeChunk(); return; } @@ -1194,5 +1178,11 @@ private static string Display(byte[] bytes, int length) BitConverter.ToString(bytes, 0, length) : BitConverter.ToString(bytes, 0, MaxDisplaySize / 2) + "-...-" + BitConverter.ToString(bytes, length - MaxDisplaySize / 2, MaxDisplaySize / 2); } + + private void ClearAndFreeChunk() + { + ClearChunk(); + FreeChunk(); + } } } diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs index ca41072b9d99b..6c05f36d046c9 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs @@ -1039,27 +1039,6 @@ public void Pooled() builder2.Free(); } - [Fact] - public void FreeNonEmptySuffixAndPrefixBlobAndEmptyOnes() - { - var b1 = PooledBlobBuilder.GetInstance(); - var b2 = PooledBlobBuilder.GetInstance(); - var b3 = PooledBlobBuilder.GetInstance(); - var b4 = PooledBlobBuilder.GetInstance(); - var b5 = PooledBlobBuilder.GetInstance(); - - b1.WriteBytes(1, 1); - b2.WriteBytes(1, 1); - b3.WriteBytes(1, 1); - - b1.LinkSuffix(b2); - b1.LinkPrefix(b3); - b1.LinkSuffix(b4); - b1.LinkPrefix(b5); - - b1.Free(); - } - private class ProperStreamRead_TestStream : TestStreamBase { private readonly byte[] _sourceArray; From 64166fd10a7e58db2a0ebf30f8a74944597fdb7e Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Fri, 5 Apr 2024 00:08:23 +0200 Subject: [PATCH 4/4] add unit tests --- .../System/Reflection/Metadata/BlobBuilder.cs | 2 +- .../tests/Metadata/BlobTests.cs | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs index f89f76e723ca7..97967bba08006 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs @@ -47,7 +47,7 @@ public partial class BlobBuilder private uint _length; private const uint IsFrozenMask = 0x80000000; - private bool IsHead => (_length & IsFrozenMask) == 0; + internal bool IsHead => (_length & IsFrozenMask) == 0; private int Length => (int)(_length & ~IsFrozenMask); private uint FrozenLength => _length | IsFrozenMask; private Span Span => _buffer.AsSpan(0, Length); diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs index 6c05f36d046c9..a04d2b6cf19ef 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/BlobTests.cs @@ -1090,5 +1090,31 @@ public void PrematureEndOfStream() AssertEx.Equal(sourceArray, builder.ToArray()); } + + [Fact] + public void LinkEmptySuffixAndPrefixShouldFreeThem() + { + var b1 = PooledBlobBuilder.GetInstance(); + var b2 = PooledBlobBuilder.GetInstance(); + var b3 = PooledBlobBuilder.GetInstance(); + var b4 = PooledBlobBuilder.GetInstance(); + var b5 = PooledBlobBuilder.GetInstance(); + + b1.WriteBytes(1, 1); + b2.WriteBytes(1, 1); + b3.WriteBytes(1, 1); + + b1.LinkSuffix(b2); + Assert.False(b2.IsHead); + + b1.LinkPrefix(b3); + Assert.False(b3.IsHead); + + b1.LinkSuffix(b4); + Assert.True(b4.IsHead); + + b1.LinkPrefix(b5); + Assert.True(b4.IsHead); + } } }