From a975e655594f254a5d77b016bb6f9fb72fc9904f Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Tue, 16 Nov 2021 09:51:28 +0100 Subject: [PATCH 01/26] Temporary ETWs --- src/Framework/MSBuildEventSource.cs | 13 +++++++++++++ src/Shared/ReuseableStringBuilder.cs | 15 ++++++++++++++- src/Shared/StringBuilderCache.cs | 16 +++++++++++++++- 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/Framework/MSBuildEventSource.cs b/src/Framework/MSBuildEventSource.cs index e32cb633fbc..54507da16dc 100644 --- a/src/Framework/MSBuildEventSource.cs +++ b/src/Framework/MSBuildEventSource.cs @@ -490,6 +490,19 @@ public void CachedSdkResolverServiceResolveSdkStop(string sdkName, string soluti WriteEvent(67, sdkName, solutionPath, projectPath, success); } + [Event(68, Keywords = Keywords.All)] + public void ReusableStringBuilderFactoryStart(int hash, int newCapacity, int oldCapacity, string type) + { + WriteEvent(68, hash, newCapacity, oldCapacity, type); + } + + [Event(69, Keywords = Keywords.All)] + public void ReusableStringBuilderFactoryStop(int hash, int returningCapacity, string type) + { + WriteEvent(69, hash, returningCapacity, type); + } + + #endregion } } diff --git a/src/Shared/ReuseableStringBuilder.cs b/src/Shared/ReuseableStringBuilder.cs index 8abf89a0093..77181e97ea5 100644 --- a/src/Shared/ReuseableStringBuilder.cs +++ b/src/Shared/ReuseableStringBuilder.cs @@ -8,6 +8,7 @@ using System.Globalization; using System.Text; using System.Threading; +using Microsoft.Build.Eventing; namespace Microsoft.Build.Shared { @@ -236,6 +237,7 @@ internal static StringBuilder Get(int capacity) #endif // Currently loaned out so return a new one returned = new StringBuilder(capacity); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity:capacity, oldCapacity:0, type:"missed"); } else if (returned.Capacity < capacity) { @@ -244,8 +246,13 @@ internal static StringBuilder Get(int capacity) #endif // It's essential we guarantee the capacity because this // may be used as a buffer to a PInvoke call. + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "reused-inflated"); returned.Capacity = capacity; } + else + { + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "reused"); + } #if DEBUG Interlocked.Increment(ref s_hits); @@ -280,14 +287,20 @@ internal static void Release(StringBuilder returningBuilder) // ErrorUtilities.VerifyThrow(handouts.TryRemove(returningBuilder, out dummy), "returned but not loaned"); returningBuilder.Clear(); // Clear before pooling - Interlocked.Exchange(ref s_sharedBuilder, returningBuilder); + var oldSharedBuilder = Interlocked.Exchange(ref s_sharedBuilder, returningBuilder); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, type: oldSharedBuilder == null ? "returned-set" : "returned-replace"); + #if DEBUG Interlocked.Increment(ref s_accepts); +#endif } else { + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, type: "discarded"); +#if DEBUG Interlocked.Increment(ref s_discards); #endif + } } diff --git a/src/Shared/StringBuilderCache.cs b/src/Shared/StringBuilderCache.cs index 9bc6ffeb15d..d81cc81bcdf 100644 --- a/src/Shared/StringBuilderCache.cs +++ b/src/Shared/StringBuilderCache.cs @@ -33,6 +33,9 @@ using System; using System.Text; +#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS +using Microsoft.Build.Eventing; +#endif namespace Microsoft.Build.Shared { @@ -59,11 +62,19 @@ public static StringBuilder Acquire(int capacity = 16 /*StringBuilder.DefaultCap { StringBuilderCache.t_cachedInstance = null; sb.Length = 0; // Equivalent of sb.Clear() that works on .Net 3.5 +#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: sb.GetHashCode(), newCapacity: capacity, oldCapacity: sb.Capacity, type: "sbc-reused"); +#endif return sb; } } } - return new StringBuilder(capacity); + + StringBuilder stringBuilder = new StringBuilder(capacity); +#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: stringBuilder.GetHashCode(), newCapacity: capacity, oldCapacity: stringBuilder.Capacity, type: "sbc-new"); +#endif + return stringBuilder; } public static void Release(StringBuilder sb) @@ -72,6 +83,9 @@ public static void Release(StringBuilder sb) { StringBuilderCache.t_cachedInstance = sb; } +#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: sb.GetHashCode(), returningCapacity: sb.Capacity, type: sb.Capacity <= MAX_BUILDER_SIZE ? "sbc-returned" : "sbc-discarded"); +#endif } public static string GetStringAndRelease(StringBuilder sb) From 8954c9a18375c79075f2b2821c9d0e9b04db96c3 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Thu, 18 Nov 2021 16:38:42 +0100 Subject: [PATCH 02/26] Set work aside --- src/Framework/MSBuildEventSource.cs | 4 ++-- src/Shared/ReuseableStringBuilder.cs | 10 ++++++---- src/Shared/StringBuilderCache.cs | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/Framework/MSBuildEventSource.cs b/src/Framework/MSBuildEventSource.cs index 54507da16dc..d9876ad1227 100644 --- a/src/Framework/MSBuildEventSource.cs +++ b/src/Framework/MSBuildEventSource.cs @@ -497,9 +497,9 @@ public void ReusableStringBuilderFactoryStart(int hash, int newCapacity, int old } [Event(69, Keywords = Keywords.All)] - public void ReusableStringBuilderFactoryStop(int hash, int returningCapacity, string type) + public void ReusableStringBuilderFactoryStop(int hash, int returningCapacity, int returningLength, string type) { - WriteEvent(69, hash, returningCapacity, type); + WriteEvent(69, hash, returningCapacity, returningLength, type); } diff --git a/src/Shared/ReuseableStringBuilder.cs b/src/Shared/ReuseableStringBuilder.cs index 77181e97ea5..5041e213a84 100644 --- a/src/Shared/ReuseableStringBuilder.cs +++ b/src/Shared/ReuseableStringBuilder.cs @@ -174,12 +174,12 @@ private static class ReuseableStringBuilderFactory /// because we could otherwise hold a huge builder indefinitely. /// This size seems reasonable for MSBuild uses (mostly expression expansion) /// - private const int MaxBuilderSize = 1024; + private const int MaxBuilderSize = 10*1024*1024; /// /// The shared builder. /// - private static StringBuilder s_sharedBuilder; + private static StringBuilder s_sharedBuilder = new(MaxBuilderSize); #if DEBUG /// @@ -273,6 +273,8 @@ internal static StringBuilder Get(int capacity) /// internal static void Release(StringBuilder returningBuilder) { + int returningLength = returningBuilder.Length; + // It's possible for someone to cause the builder to // enlarge to such an extent that this static field // would be a leak. To avoid that, only accept @@ -288,7 +290,7 @@ internal static void Release(StringBuilder returningBuilder) returningBuilder.Clear(); // Clear before pooling var oldSharedBuilder = Interlocked.Exchange(ref s_sharedBuilder, returningBuilder); - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, type: oldSharedBuilder == null ? "returned-set" : "returned-replace"); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: oldSharedBuilder == null ? "returned-set" : "returned-replace"); #if DEBUG Interlocked.Increment(ref s_accepts); @@ -296,7 +298,7 @@ internal static void Release(StringBuilder returningBuilder) } else { - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, type: "discarded"); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: "discarded"); #if DEBUG Interlocked.Increment(ref s_discards); #endif diff --git a/src/Shared/StringBuilderCache.cs b/src/Shared/StringBuilderCache.cs index d81cc81bcdf..c5663d2d3e6 100644 --- a/src/Shared/StringBuilderCache.cs +++ b/src/Shared/StringBuilderCache.cs @@ -84,7 +84,7 @@ public static void Release(StringBuilder sb) StringBuilderCache.t_cachedInstance = sb; } #if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: sb.GetHashCode(), returningCapacity: sb.Capacity, type: sb.Capacity <= MAX_BUILDER_SIZE ? "sbc-returned" : "sbc-discarded"); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: sb.GetHashCode(), returningCapacity: sb.Capacity, returningLength: sb.Length, type: sb.Capacity <= MAX_BUILDER_SIZE ? "sbc-returned" : "sbc-discarded"); #endif } From 2dbda742a4fc909db8dc3ef93cde61b518e63e06 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Tue, 23 Nov 2021 16:26:00 +0100 Subject: [PATCH 03/26] Enlarge ReusableStringBuilderFactory limits and slightly change logic. --- src/Shared/ReuseableStringBuilder.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Shared/ReuseableStringBuilder.cs b/src/Shared/ReuseableStringBuilder.cs index 5041e213a84..28baa3b23d7 100644 --- a/src/Shared/ReuseableStringBuilder.cs +++ b/src/Shared/ReuseableStringBuilder.cs @@ -172,9 +172,9 @@ private static class ReuseableStringBuilderFactory /// /// Made up limit beyond which we won't share the builder /// because we could otherwise hold a huge builder indefinitely. - /// This size seems reasonable for MSBuild uses (mostly expression expansion) + /// This was picked empirically so 95% percentile of data String Builder needs is reused. /// - private const int MaxBuilderSize = 10*1024*1024; + private const int MaxBuilderSize = 512 * 1024; // 0.5 MB /// /// The shared builder. @@ -236,7 +236,7 @@ internal static StringBuilder Get(int capacity) Interlocked.Increment(ref s_misses); #endif // Currently loaned out so return a new one - returned = new StringBuilder(capacity); + returned = new StringBuilder(Math.Min(MaxBuilderSize, capacity)); MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity:capacity, oldCapacity:0, type:"missed"); } else if (returned.Capacity < capacity) @@ -284,7 +284,7 @@ internal static void Release(StringBuilder returningBuilder) // (or we refuse it here because it's too big) the next user will // get given a new one, and then return it soon after. // So the shared builder will be "replaced". - if (returningBuilder.Capacity < MaxBuilderSize) + if (returningBuilder.Capacity <= MaxBuilderSize) { // ErrorUtilities.VerifyThrow(handouts.TryRemove(returningBuilder, out dummy), "returned but not loaned"); returningBuilder.Clear(); // Clear before pooling From 9ea16234501cae6c2a40309eac10d1bc32a1d28d Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 24 Nov 2021 14:07:36 +0100 Subject: [PATCH 04/26] Move string builder caches into Framework --- ...Microsoft.Build.Engine.OM.UnitTests.csproj | 3 - src/Build/Microsoft.Build.csproj | 4 - ...Microsoft.Build.Framework.UnitTests.csproj | 1 - src/Framework/ReuseableStringBuilder.cs | 329 ++++++++++++++++++ src/Framework/StringBuilderCache.cs | 98 ++++++ src/Shared/EscapingUtilities.cs | 1 + src/Shared/ReuseableStringBuilder.cs | 3 +- src/Tasks/Microsoft.Build.Tasks.csproj | 7 - .../Microsoft.Build.Utilities.csproj | 8 +- 9 files changed, 431 insertions(+), 23 deletions(-) create mode 100644 src/Framework/ReuseableStringBuilder.cs create mode 100644 src/Framework/StringBuilderCache.cs diff --git a/src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj b/src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj index 36c8a20a7f0..00221821ec4 100644 --- a/src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj +++ b/src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj @@ -77,9 +77,6 @@ true - - True - true diff --git a/src/Build/Microsoft.Build.csproj b/src/Build/Microsoft.Build.csproj index cbfee6c66db..19c39093f3f 100644 --- a/src/Build/Microsoft.Build.csproj +++ b/src/Build/Microsoft.Build.csproj @@ -99,9 +99,6 @@ Collections\ReadOnlyEmptyCollection.cs - - True - @@ -140,7 +137,6 @@ true - diff --git a/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj b/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj index 2ce98b0c404..da03486b433 100644 --- a/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj +++ b/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj @@ -41,7 +41,6 @@ - diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs new file mode 100644 index 00000000000..20353dbf74b --- /dev/null +++ b/src/Framework/ReuseableStringBuilder.cs @@ -0,0 +1,329 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; +using System.Globalization; +using System.Text; +using System.Threading; +using Microsoft.Build.Eventing; + +namespace Microsoft.Build.Framework +{ + /// + /// A StringBuilder lookalike that reuses its internal storage. + /// + /// + /// This class is being deprecated in favor of SpanBasedStringBuilder in StringTools. Avoid adding more uses. + /// + internal sealed class ReuseableStringBuilder : IDisposable + { + /// + /// Captured string builder. + /// + private StringBuilder _borrowedBuilder; + + /// + /// Capacity to initialize the builder with. + /// + private int _capacity; + + /// + /// Create a new builder, under the covers wrapping a reused one. + /// + internal ReuseableStringBuilder(int capacity = 16) // StringBuilder default is 16 + { + _capacity = capacity; + + // lazy initialization of the builder + } + + /// + /// The length of the target. + /// + public int Length + { + get { return (_borrowedBuilder == null) ? 0 : _borrowedBuilder.Length; } + set + { + LazyPrepare(); + _borrowedBuilder.Length = value; + } + } + + /// + /// Convert to a string. + /// + public override string ToString() + { + if (_borrowedBuilder == null) + { + return String.Empty; + } + + return _borrowedBuilder.ToString(); + } + + /// + /// Dispose, indicating you are done with this builder. + /// + void IDisposable.Dispose() + { + if (_borrowedBuilder != null) + { + ReuseableStringBuilderFactory.Release(_borrowedBuilder); + _borrowedBuilder = null; + _capacity = -1; + } + } + + /// + /// Append a character. + /// + internal ReuseableStringBuilder Append(char value) + { + LazyPrepare(); + _borrowedBuilder.Append(value); + return this; + } + + /// + /// Append a string. + /// + internal ReuseableStringBuilder Append(string value) + { + LazyPrepare(); + _borrowedBuilder.Append(value); + return this; + } + + /// + /// Append a substring. + /// + internal ReuseableStringBuilder Append(string value, int startIndex, int count) + { + LazyPrepare(); + _borrowedBuilder.Append(value, startIndex, count); + return this; + } + + public ReuseableStringBuilder AppendSeparated(char separator, ICollection strings) + { + LazyPrepare(); + + var separatorsRemaining = strings.Count - 1; + + foreach (var s in strings) + { + _borrowedBuilder.Append(s); + + if (separatorsRemaining > 0) + { + _borrowedBuilder.Append(separator); + } + + separatorsRemaining--; + } + + return this; + } + + public ReuseableStringBuilder Clear() + { + LazyPrepare(); + _borrowedBuilder.Clear(); + return this; + } + + /// + /// Remove a substring. + /// + internal ReuseableStringBuilder Remove(int startIndex, int length) + { + LazyPrepare(); + _borrowedBuilder.Remove(startIndex, length); + return this; + } + + /// + /// Grab a backing builder if necessary. + /// + private void LazyPrepare() + { + if (_borrowedBuilder == null) + { + // TODO: enable once rebased to ErrorUtilities in Framework + //ErrorUtilities.VerifyThrow(_capacity != -1, "Reusing after dispose"); + + _borrowedBuilder = ReuseableStringBuilderFactory.Get(_capacity); + } + } + + /// + /// A utility class that mediates access to a shared string builder. + /// + /// + /// If this shared builder is highly contended, this class could add + /// a second one and try both in turn. + /// + private static class ReuseableStringBuilderFactory + { + /// + /// Made up limit beyond which we won't share the builder + /// because we could otherwise hold a huge builder indefinitely. + /// This was picked empirically so 95% percentile of data String Builder needs is reused. + /// + private const int MaxBuilderSize = 512 * 1024; // 0.5 MB + + /// + /// The shared builder. + /// + private static StringBuilder s_sharedBuilder = new(MaxBuilderSize); + +#if DEBUG + /// + /// Count of successful reuses + /// + private static int s_hits = 0; + + /// + /// Count of failed reuses - a new builder was created + /// + private static int s_misses = 0; + + /// + /// Count of times the builder capacity was raised to satisfy the caller's request + /// + private static int s_upsizes = 0; + + /// + /// Count of times the returned builder was discarded because it was too large + /// + private static int s_discards = 0; + + /// + /// Count of times the builder was returned. + /// + private static int s_accepts = 0; + + /// + /// Aggregate capacity saved (aggregate midpoints of requested and returned) + /// + private static int s_saved = 0; + + /// + /// Callstacks of those handed out and not returned yet + /// + private static ConcurrentDictionary s_handouts = new ConcurrentDictionary(); +#endif + /// + /// Obtains a string builder which may or may not already + /// have been used. + /// Never returns null. + /// + internal static StringBuilder Get(int capacity) + { +#if DEBUG + bool missed = false; +#endif + var returned = Interlocked.Exchange(ref s_sharedBuilder, null); + + if (returned == null) + { +#if DEBUG + missed = true; + Interlocked.Increment(ref s_misses); +#endif + // Currently loaned out so return a new one + returned = new StringBuilder(Math.Min(MaxBuilderSize, capacity)); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity:capacity, oldCapacity:0, type:"missed"); + } + else if (returned.Capacity < capacity) + { +#if DEBUG + Interlocked.Increment(ref s_upsizes); +#endif + // It's essential we guarantee the capacity because this + // may be used as a buffer to a PInvoke call. + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "reused-inflated"); + returned.Capacity = capacity; + } + else + { + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "reused"); + } + +#if DEBUG + Interlocked.Increment(ref s_hits); + + if (!missed) + { + Interlocked.Add(ref s_saved, (capacity + returned.Capacity) / 2); + } + + // handouts.TryAdd(returned, Environment.StackTrace); +#endif + return returned; + } + + /// + /// Returns the shared builder for the next caller to use. + /// ** CALLERS, DO NOT USE THE BUILDER AFTER RELEASING IT HERE! ** + /// + internal static void Release(StringBuilder returningBuilder) + { + int returningLength = returningBuilder.Length; + + // It's possible for someone to cause the builder to + // enlarge to such an extent that this static field + // would be a leak. To avoid that, only accept + // the builder if it's no more than a certain size. + // + // If some code has a bug and forgets to return their builder + // (or we refuse it here because it's too big) the next user will + // get given a new one, and then return it soon after. + // So the shared builder will be "replaced". + if (returningBuilder.Capacity <= MaxBuilderSize) + { + // ErrorUtilities.VerifyThrow(handouts.TryRemove(returningBuilder, out dummy), "returned but not loaned"); + returningBuilder.Clear(); // Clear before pooling + + var oldSharedBuilder = Interlocked.Exchange(ref s_sharedBuilder, returningBuilder); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: oldSharedBuilder == null ? "returned-set" : "returned-replace"); + +#if DEBUG + Interlocked.Increment(ref s_accepts); +#endif + } + else + { + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: "discarded"); +#if DEBUG + Interlocked.Increment(ref s_discards); +#endif + + } + } + +#if DEBUG + /// + /// Debugging dumping + /// + [SuppressMessage("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode", Justification = "Handy helper method that can be used to annotate ReuseableStringBuilder when debugging it, but is not hooked up usually for the sake of perf.")] + [SuppressMessage("Microsoft.Usage", "CA1806:DoNotIgnoreMethodResults", MessageId = "System.String.Format(System.IFormatProvider,System.String,System.Object[])", Justification = "Handy string that can be used to annotate ReuseableStringBuilder when debugging it, but is not hooked up usually.")] + internal static void DumpUnreturned() + { + String.Format(CultureInfo.CurrentUICulture, "{0} Hits of which\n {1} Misses (was on loan)\n {2} Upsizes (needed bigger) \n\n{3} Returns=\n{4} Discards (returned too large)+\n {5} Accepts\n\n{6} estimated bytes saved", s_hits, s_misses, s_upsizes, s_discards + s_accepts, s_discards, s_accepts, s_saved); + + Console.WriteLine("Unreturned string builders were allocated here:"); + foreach (var entry in s_handouts.Values) + { + Console.WriteLine(entry + "\n"); + } + } +#endif + } + } +} diff --git a/src/Framework/StringBuilderCache.cs b/src/Framework/StringBuilderCache.cs new file mode 100644 index 00000000000..a5d334aed41 --- /dev/null +++ b/src/Framework/StringBuilderCache.cs @@ -0,0 +1,98 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +/*============================================================ +** +** +** Purpose: provide a cached reusable instance of StringBuilder +** per thread it's an optimization that reduces the +** number of instances constructed and collected. +** +** Acquire - is used to get a string builder to use of a +** particular size. It can be called any number of +** times, if a StringBuilder is in the cache then +** it will be returned and the cache emptied. +** subsequent calls will return a new StringBuilder. +** +** A StringBuilder instance is cached in +** Thread Local Storage and so there is one per thread +** +** Release - Place the specified builder in the cache if it is +** not too big. +** The StringBuilder should not be used after it has +** been released. +** Unbalanced Releases are perfectly acceptable. It +** will merely cause the runtime to create a new +** StringBuilder next time Acquire is called. +** +** GetStringAndRelease +** - ToString() the StringBuilder, Release it to the +** cache and return the resulting string +** +===========================================================*/ + +using System; +using System.Text; +#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS +using Microsoft.Build.Eventing; +#endif + +namespace Microsoft.Build.Framework +{ + internal static class StringBuilderCache + { + // The value 360 was chosen in discussion with performance experts as a compromise between using + // as little memory (per thread) as possible and still covering a large part of short-lived + // StringBuilder creations on the startup path of VS designers. + private const int MAX_BUILDER_SIZE = 360; + + [ThreadStatic] + private static StringBuilder t_cachedInstance; + + public static StringBuilder Acquire(int capacity = 16 /*StringBuilder.DefaultCapacity*/) + { + if (capacity <= MAX_BUILDER_SIZE) + { + StringBuilder sb = StringBuilderCache.t_cachedInstance; + if (sb != null) + { + // Avoid StringBuilder block fragmentation by getting a new StringBuilder + // when the requested size is larger than the current capacity + if (capacity <= sb.Capacity) + { + StringBuilderCache.t_cachedInstance = null; + sb.Length = 0; // Equivalent of sb.Clear() that works on .Net 3.5 +#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: sb.GetHashCode(), newCapacity: capacity, oldCapacity: sb.Capacity, type: "sbc-reused"); +#endif + return sb; + } + } + } + + StringBuilder stringBuilder = new StringBuilder(capacity); +#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: stringBuilder.GetHashCode(), newCapacity: capacity, oldCapacity: stringBuilder.Capacity, type: "sbc-new"); +#endif + return stringBuilder; + } + + public static void Release(StringBuilder sb) + { + if (sb.Capacity <= MAX_BUILDER_SIZE) + { + StringBuilderCache.t_cachedInstance = sb; + } +#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: sb.GetHashCode(), returningCapacity: sb.Capacity, returningLength: sb.Length, type: sb.Capacity <= MAX_BUILDER_SIZE ? "sbc-returned" : "sbc-discarded"); +#endif + } + + public static string GetStringAndRelease(StringBuilder sb) + { + string result = sb.ToString(); + Release(sb); + return result; + } + } +} diff --git a/src/Shared/EscapingUtilities.cs b/src/Shared/EscapingUtilities.cs index 309e39a2988..723a4df0139 100644 --- a/src/Shared/EscapingUtilities.cs +++ b/src/Shared/EscapingUtilities.cs @@ -6,6 +6,7 @@ using System.Globalization; using System.Text; +using Microsoft.Build.Framework; using Microsoft.NET.StringTools; namespace Microsoft.Build.Shared diff --git a/src/Shared/ReuseableStringBuilder.cs b/src/Shared/ReuseableStringBuilder.cs index 28baa3b23d7..4a113251de9 100644 --- a/src/Shared/ReuseableStringBuilder.cs +++ b/src/Shared/ReuseableStringBuilder.cs @@ -154,7 +154,8 @@ private void LazyPrepare() { if (_borrowedBuilder == null) { - ErrorUtilities.VerifyThrow(_capacity != -1, "Reusing after dispose"); + // TODO: merge when rebaed to Rainer changes + //ErrorUtilities.VerifyThrow(_capacity != -1, "Reusing after dispose"); _borrowedBuilder = ReuseableStringBuilderFactory.Get(_capacity); } diff --git a/src/Tasks/Microsoft.Build.Tasks.csproj b/src/Tasks/Microsoft.Build.Tasks.csproj index 19b5dde493f..85672c4c284 100644 --- a/src/Tasks/Microsoft.Build.Tasks.csproj +++ b/src/Tasks/Microsoft.Build.Tasks.csproj @@ -98,13 +98,6 @@ RegistryHelper.cs true - - StringBuilderCache.cs - true - - - ReuseableStringBuilder.cs - StrongNameHelpers.cs diff --git a/src/Utilities/Microsoft.Build.Utilities.csproj b/src/Utilities/Microsoft.Build.Utilities.csproj index 065b9fc3233..4406635c278 100644 --- a/src/Utilities/Microsoft.Build.Utilities.csproj +++ b/src/Utilities/Microsoft.Build.Utilities.csproj @@ -1,4 +1,4 @@ - + @@ -131,12 +131,6 @@ Shared\ResourceUtilities.cs - - Shared\ReuseableStringBuilder.cs - - - Shared\StringBuilderCache.cs - Shared\TaskLoggingHelper.cs From 176527e83d2033671e2b00571b74b2e98aa50f1f Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Thu, 25 Nov 2021 12:05:22 +0100 Subject: [PATCH 05/26] Finish moving ReuseableStringBuilder into framework --- src/Framework/ReuseableStringBuilder.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 20353dbf74b..97f47a6f54b 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -154,8 +154,7 @@ private void LazyPrepare() { if (_borrowedBuilder == null) { - // TODO: enable once rebased to ErrorUtilities in Framework - //ErrorUtilities.VerifyThrow(_capacity != -1, "Reusing after dispose"); + FrameworkErrorUtilities.VerifyThrow(_capacity != -1, "Reusing after dispose"); _borrowedBuilder = ReuseableStringBuilderFactory.Get(_capacity); } From 024aba447e7443e73d39701af05288b6e5da67f3 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Thu, 25 Nov 2021 20:26:17 +0100 Subject: [PATCH 06/26] Capacity bracketing --- src/Framework/MSBuildEventSource.cs | 7 +- src/Framework/ReuseableStringBuilder.cs | 173 ++++++++++++------------ 2 files changed, 90 insertions(+), 90 deletions(-) diff --git a/src/Framework/MSBuildEventSource.cs b/src/Framework/MSBuildEventSource.cs index d9876ad1227..e93c77a204a 100644 --- a/src/Framework/MSBuildEventSource.cs +++ b/src/Framework/MSBuildEventSource.cs @@ -502,7 +502,12 @@ public void ReusableStringBuilderFactoryStop(int hash, int returningCapacity, in WriteEvent(69, hash, returningCapacity, returningLength, type); } + [Event(70, Keywords = Keywords.All)] + public void ReusableStringBuilderFactoryReplace(int oldHash, int newHash) + { + WriteEvent(70, oldHash, newHash); + } - #endregion +#endregion } } diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 97f47a6f54b..945846a4604 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -2,10 +2,8 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; -using System.Collections.Concurrent; using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; -using System.Globalization; +using System.Diagnostics; using System.Text; using System.Threading; using Microsoft.Build.Eventing; @@ -25,6 +23,11 @@ internal sealed class ReuseableStringBuilder : IDisposable /// private StringBuilder _borrowedBuilder; + /// + /// Capacity of borrowed string builder at the time of borrowing. + /// + private int _borrowedWithCapacity; + /// /// Capacity to initialize the builder with. /// @@ -73,7 +76,7 @@ void IDisposable.Dispose() { if (_borrowedBuilder != null) { - ReuseableStringBuilderFactory.Release(_borrowedBuilder); + ReuseableStringBuilderFactory.Release(this); _borrowedBuilder = null; _capacity = -1; } @@ -157,6 +160,7 @@ private void LazyPrepare() FrameworkErrorUtilities.VerifyThrow(_capacity != -1, "Reusing after dispose"); _borrowedBuilder = ReuseableStringBuilderFactory.Get(_capacity); + _borrowedWithCapacity = _borrowedBuilder.Capacity; } } @@ -172,51 +176,40 @@ private static class ReuseableStringBuilderFactory /// /// Made up limit beyond which we won't share the builder /// because we could otherwise hold a huge builder indefinitely. - /// This was picked empirically so 95% percentile of data String Builder needs is reused. - /// - private const int MaxBuilderSize = 512 * 1024; // 0.5 MB - - /// - /// The shared builder. + /// This was picked empirically to save at least 95% of allocated data size. + /// This constant has to exactly 2^n (power of 2) where n = 4 ... 32 /// - private static StringBuilder s_sharedBuilder = new(MaxBuilderSize); + /// + /// This constant might looks huge, but rather that lowering this constant, + /// we shall focus on eliminating of code which requires to create such huge strings. + /// + private const int MaxBuilderSizeBytes = 2 * 1024 * 1024; // ~1M chars + private const int MaxBuilderSizeCapacity = MaxBuilderSizeBytes / 2; -#if DEBUG - /// - /// Count of successful reuses - /// - private static int s_hits = 0; + private static readonly IReadOnlyList s_capacityBrackets; - /// - /// Count of failed reuses - a new builder was created - /// - private static int s_misses = 0; - - /// - /// Count of times the builder capacity was raised to satisfy the caller's request - /// - private static int s_upsizes = 0; + static ReuseableStringBuilderFactory() + { + var brackets = new List(); - /// - /// Count of times the returned builder was discarded because it was too large - /// - private static int s_discards = 0; + int bytes = 0x200; // Minimal capacity is 256 (512 bytes) as this was, according to captured traces, mean required capacity + while (bytes <= MaxBuilderSizeBytes) + { + // Allocation of arrays is optimized in byte[bytes] => bytes = 2^n. + // StringBuilder allocates chars[capacity] and each char is 2 bytes so lets have capacity brackets computed as `bytes/2` + brackets.Add(bytes/2); + bytes <<= 1; + } + Debug.Assert((bytes >> 1) == MaxBuilderSizeBytes, "MaxBuilderSizeBytes has to be 2^n (power of 2)"); - /// - /// Count of times the builder was returned. - /// - private static int s_accepts = 0; + s_capacityBrackets = brackets; + } /// - /// Aggregate capacity saved (aggregate midpoints of requested and returned) + /// The shared builder. /// - private static int s_saved = 0; + private static StringBuilder s_sharedBuilder; - /// - /// Callstacks of those handed out and not returned yet - /// - private static ConcurrentDictionary s_handouts = new ConcurrentDictionary(); -#endif /// /// Obtains a string builder which may or may not already /// have been used. @@ -224,46 +217,34 @@ private static class ReuseableStringBuilderFactory /// internal static StringBuilder Get(int capacity) { -#if DEBUG - bool missed = false; -#endif var returned = Interlocked.Exchange(ref s_sharedBuilder, null); if (returned == null) { + // Currently loaned out so return a new one with capacity in given bracket. + // If user wants bigger capacity that maximum capacity respect it. + returned = new StringBuilder(SelectBracketedCapacity(capacity)); #if DEBUG - missed = true; - Interlocked.Increment(ref s_misses); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity:capacity, oldCapacity:0, type:"miss"); #endif - // Currently loaned out so return a new one - returned = new StringBuilder(Math.Min(MaxBuilderSize, capacity)); - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity:capacity, oldCapacity:0, type:"missed"); } else if (returned.Capacity < capacity) { -#if DEBUG - Interlocked.Increment(ref s_upsizes); -#endif // It's essential we guarantee the capacity because this // may be used as a buffer to a PInvoke call. - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "reused-inflated"); - returned.Capacity = capacity; + int newCapacity = SelectBracketedCapacity(capacity); +#if DEBUG + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: newCapacity, oldCapacity: returned.Capacity, type: "miss-need-bigger"); +#endif + // Let the current StringBuilder be collected and create new with bracketed capacity. This way it allocates only char[newCapacity] + // otherwise it would allocate char[new_capacity_of_last_chunk] (in set_Capacity) and char[newCapacity] (in Clear). + returned = new StringBuilder(SelectBracketedCapacity(newCapacity)); } else { - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "reused"); - } - -#if DEBUG - Interlocked.Increment(ref s_hits); - - if (!missed) - { - Interlocked.Add(ref s_saved, (capacity + returned.Capacity) / 2); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "hit"); } - // handouts.TryAdd(returned, Environment.StackTrace); -#endif return returned; } @@ -271,8 +252,9 @@ internal static StringBuilder Get(int capacity) /// Returns the shared builder for the next caller to use. /// ** CALLERS, DO NOT USE THE BUILDER AFTER RELEASING IT HERE! ** /// - internal static void Release(StringBuilder returningBuilder) + internal static void Release(ReuseableStringBuilder returning) { + StringBuilder returningBuilder = returning._borrowedBuilder; int returningLength = returningBuilder.Length; // It's possible for someone to cause the builder to @@ -284,45 +266,58 @@ internal static void Release(StringBuilder returningBuilder) // (or we refuse it here because it's too big) the next user will // get given a new one, and then return it soon after. // So the shared builder will be "replaced". - if (returningBuilder.Capacity <= MaxBuilderSize) + if (returningBuilder.Capacity > MaxBuilderSizeCapacity) { - // ErrorUtilities.VerifyThrow(handouts.TryRemove(returningBuilder, out dummy), "returned but not loaned"); - returningBuilder.Clear(); // Clear before pooling - - var oldSharedBuilder = Interlocked.Exchange(ref s_sharedBuilder, returningBuilder); - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: oldSharedBuilder == null ? "returned-set" : "returned-replace"); - + // In order to free memory usage by huge string builder, do not pull this one and let it be collected. #if DEBUG - Interlocked.Increment(ref s_accepts); + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: "discard"); #endif } else { - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: "discarded"); + if (returningBuilder.Capacity != returning._borrowedWithCapacity) + { + Debug.Assert(returningBuilder.Capacity > returning._borrowedWithCapacity, "Capacity can only increase"); + + // This builder used more that pre-allocated capacity bracket. + // Let this builder be collected and put new builder, with reflecting bracket capacity, into the pool. + // If we would just return this builder into pool as is, it would allocated new array[capacity] anyway (current implementation of returningBuilder.Clear() does it) + // and that could lead to unpredictable amount of LOH allocations and eventual LOH fragmentation. + // Bellow implementation have predictable max Log2(MaxBuilderSizeBytes) string builder array re-allocations during whole process lifetime - unless MaxBuilderSizeCapacity is reached frequently. + int newCapacity = SelectBracketedCapacity(returningBuilder.Capacity); + returningBuilder = new StringBuilder(newCapacity); + } + + returningBuilder.Clear(); // Clear before pooling + + var oldSharedBuilder = Interlocked.Exchange(ref s_sharedBuilder, returningBuilder); + if (oldSharedBuilder != null) + { #if DEBUG - Interlocked.Increment(ref s_discards); + // This can identify in-proper usage from multiple thread or bug in code - forgotten Dispose. + // Look at stack traces of ETW events with reporter string builder hashes. + MSBuildEventSource.Log.ReusableStringBuilderFactoryReplace(oldHash: oldSharedBuilder.GetHashCode(), newHash: returningBuilder.GetHashCode()); +#endif + } +#if DEBUG + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: returning._borrowedBuilder != returningBuilder ? "return-new" : "return"); #endif - } } -#if DEBUG - /// - /// Debugging dumping - /// - [SuppressMessage("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode", Justification = "Handy helper method that can be used to annotate ReuseableStringBuilder when debugging it, but is not hooked up usually for the sake of perf.")] - [SuppressMessage("Microsoft.Usage", "CA1806:DoNotIgnoreMethodResults", MessageId = "System.String.Format(System.IFormatProvider,System.String,System.Object[])", Justification = "Handy string that can be used to annotate ReuseableStringBuilder when debugging it, but is not hooked up usually.")] - internal static void DumpUnreturned() + private static int SelectBracketedCapacity(int requiredCapacity) { - String.Format(CultureInfo.CurrentUICulture, "{0} Hits of which\n {1} Misses (was on loan)\n {2} Upsizes (needed bigger) \n\n{3} Returns=\n{4} Discards (returned too large)+\n {5} Accepts\n\n{6} estimated bytes saved", s_hits, s_misses, s_upsizes, s_discards + s_accepts, s_discards, s_accepts, s_saved); - - Console.WriteLine("Unreturned string builders were allocated here:"); - foreach (var entry in s_handouts.Values) + foreach (int bracket in s_capacityBrackets) { - Console.WriteLine(entry + "\n"); + if (requiredCapacity <= bracket) + { + return bracket; + } } + + // If user wants bigger capacity than maximum respect it. It could be as buffer in p/invoke. + return requiredCapacity; } -#endif } } } From f78ddd47b5aa3b0cdb2205f2b1489a076ab95460 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Thu, 25 Nov 2021 22:29:56 +0100 Subject: [PATCH 07/26] Removing files from shared --- src/MSBuild/MSBuild.csproj | 3 - src/MSBuildTaskHost/MSBuildTaskHost.csproj | 2 +- src/Shared/ReuseableStringBuilder.cs | 329 ------------------ src/Shared/StringBuilderCache.cs | 98 ------ .../Microsoft.Build.UnitTests.Shared.csproj | 3 - 5 files changed, 1 insertion(+), 434 deletions(-) delete mode 100644 src/Shared/ReuseableStringBuilder.cs delete mode 100644 src/Shared/StringBuilderCache.cs diff --git a/src/MSBuild/MSBuild.csproj b/src/MSBuild/MSBuild.csproj index 4361f23bd06..9305e18c7ff 100644 --- a/src/MSBuild/MSBuild.csproj +++ b/src/MSBuild/MSBuild.csproj @@ -152,9 +152,6 @@ true - - true - true diff --git a/src/MSBuildTaskHost/MSBuildTaskHost.csproj b/src/MSBuildTaskHost/MSBuildTaskHost.csproj index a2e79399940..cc9f10fdb27 100644 --- a/src/MSBuildTaskHost/MSBuildTaskHost.csproj +++ b/src/MSBuildTaskHost/MSBuildTaskHost.csproj @@ -139,7 +139,7 @@ ResourceUtilities.cs - + StringBuilderCache.cs diff --git a/src/Shared/ReuseableStringBuilder.cs b/src/Shared/ReuseableStringBuilder.cs deleted file mode 100644 index 4a113251de9..00000000000 --- a/src/Shared/ReuseableStringBuilder.cs +++ /dev/null @@ -1,329 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -using System; -using System.Collections.Concurrent; -using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; -using System.Globalization; -using System.Text; -using System.Threading; -using Microsoft.Build.Eventing; - -namespace Microsoft.Build.Shared -{ - /// - /// A StringBuilder lookalike that reuses its internal storage. - /// - /// - /// This class is being deprecated in favor of SpanBasedStringBuilder in StringTools. Avoid adding more uses. - /// - internal sealed class ReuseableStringBuilder : IDisposable - { - /// - /// Captured string builder. - /// - private StringBuilder _borrowedBuilder; - - /// - /// Capacity to initialize the builder with. - /// - private int _capacity; - - /// - /// Create a new builder, under the covers wrapping a reused one. - /// - internal ReuseableStringBuilder(int capacity = 16) // StringBuilder default is 16 - { - _capacity = capacity; - - // lazy initialization of the builder - } - - /// - /// The length of the target. - /// - public int Length - { - get { return (_borrowedBuilder == null) ? 0 : _borrowedBuilder.Length; } - set - { - LazyPrepare(); - _borrowedBuilder.Length = value; - } - } - - /// - /// Convert to a string. - /// - public override string ToString() - { - if (_borrowedBuilder == null) - { - return String.Empty; - } - - return _borrowedBuilder.ToString(); - } - - /// - /// Dispose, indicating you are done with this builder. - /// - void IDisposable.Dispose() - { - if (_borrowedBuilder != null) - { - ReuseableStringBuilderFactory.Release(_borrowedBuilder); - _borrowedBuilder = null; - _capacity = -1; - } - } - - /// - /// Append a character. - /// - internal ReuseableStringBuilder Append(char value) - { - LazyPrepare(); - _borrowedBuilder.Append(value); - return this; - } - - /// - /// Append a string. - /// - internal ReuseableStringBuilder Append(string value) - { - LazyPrepare(); - _borrowedBuilder.Append(value); - return this; - } - - /// - /// Append a substring. - /// - internal ReuseableStringBuilder Append(string value, int startIndex, int count) - { - LazyPrepare(); - _borrowedBuilder.Append(value, startIndex, count); - return this; - } - - public ReuseableStringBuilder AppendSeparated(char separator, ICollection strings) - { - LazyPrepare(); - - var separatorsRemaining = strings.Count - 1; - - foreach (var s in strings) - { - _borrowedBuilder.Append(s); - - if (separatorsRemaining > 0) - { - _borrowedBuilder.Append(separator); - } - - separatorsRemaining--; - } - - return this; - } - - public ReuseableStringBuilder Clear() - { - LazyPrepare(); - _borrowedBuilder.Clear(); - return this; - } - - /// - /// Remove a substring. - /// - internal ReuseableStringBuilder Remove(int startIndex, int length) - { - LazyPrepare(); - _borrowedBuilder.Remove(startIndex, length); - return this; - } - - /// - /// Grab a backing builder if necessary. - /// - private void LazyPrepare() - { - if (_borrowedBuilder == null) - { - // TODO: merge when rebaed to Rainer changes - //ErrorUtilities.VerifyThrow(_capacity != -1, "Reusing after dispose"); - - _borrowedBuilder = ReuseableStringBuilderFactory.Get(_capacity); - } - } - - /// - /// A utility class that mediates access to a shared string builder. - /// - /// - /// If this shared builder is highly contended, this class could add - /// a second one and try both in turn. - /// - private static class ReuseableStringBuilderFactory - { - /// - /// Made up limit beyond which we won't share the builder - /// because we could otherwise hold a huge builder indefinitely. - /// This was picked empirically so 95% percentile of data String Builder needs is reused. - /// - private const int MaxBuilderSize = 512 * 1024; // 0.5 MB - - /// - /// The shared builder. - /// - private static StringBuilder s_sharedBuilder = new(MaxBuilderSize); - -#if DEBUG - /// - /// Count of successful reuses - /// - private static int s_hits = 0; - - /// - /// Count of failed reuses - a new builder was created - /// - private static int s_misses = 0; - - /// - /// Count of times the builder capacity was raised to satisfy the caller's request - /// - private static int s_upsizes = 0; - - /// - /// Count of times the returned builder was discarded because it was too large - /// - private static int s_discards = 0; - - /// - /// Count of times the builder was returned. - /// - private static int s_accepts = 0; - - /// - /// Aggregate capacity saved (aggregate midpoints of requested and returned) - /// - private static int s_saved = 0; - - /// - /// Callstacks of those handed out and not returned yet - /// - private static ConcurrentDictionary s_handouts = new ConcurrentDictionary(); -#endif - /// - /// Obtains a string builder which may or may not already - /// have been used. - /// Never returns null. - /// - internal static StringBuilder Get(int capacity) - { -#if DEBUG - bool missed = false; -#endif - var returned = Interlocked.Exchange(ref s_sharedBuilder, null); - - if (returned == null) - { -#if DEBUG - missed = true; - Interlocked.Increment(ref s_misses); -#endif - // Currently loaned out so return a new one - returned = new StringBuilder(Math.Min(MaxBuilderSize, capacity)); - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity:capacity, oldCapacity:0, type:"missed"); - } - else if (returned.Capacity < capacity) - { -#if DEBUG - Interlocked.Increment(ref s_upsizes); -#endif - // It's essential we guarantee the capacity because this - // may be used as a buffer to a PInvoke call. - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "reused-inflated"); - returned.Capacity = capacity; - } - else - { - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "reused"); - } - -#if DEBUG - Interlocked.Increment(ref s_hits); - - if (!missed) - { - Interlocked.Add(ref s_saved, (capacity + returned.Capacity) / 2); - } - - // handouts.TryAdd(returned, Environment.StackTrace); -#endif - return returned; - } - - /// - /// Returns the shared builder for the next caller to use. - /// ** CALLERS, DO NOT USE THE BUILDER AFTER RELEASING IT HERE! ** - /// - internal static void Release(StringBuilder returningBuilder) - { - int returningLength = returningBuilder.Length; - - // It's possible for someone to cause the builder to - // enlarge to such an extent that this static field - // would be a leak. To avoid that, only accept - // the builder if it's no more than a certain size. - // - // If some code has a bug and forgets to return their builder - // (or we refuse it here because it's too big) the next user will - // get given a new one, and then return it soon after. - // So the shared builder will be "replaced". - if (returningBuilder.Capacity <= MaxBuilderSize) - { - // ErrorUtilities.VerifyThrow(handouts.TryRemove(returningBuilder, out dummy), "returned but not loaned"); - returningBuilder.Clear(); // Clear before pooling - - var oldSharedBuilder = Interlocked.Exchange(ref s_sharedBuilder, returningBuilder); - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: oldSharedBuilder == null ? "returned-set" : "returned-replace"); - -#if DEBUG - Interlocked.Increment(ref s_accepts); -#endif - } - else - { - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: "discarded"); -#if DEBUG - Interlocked.Increment(ref s_discards); -#endif - - } - } - -#if DEBUG - /// - /// Debugging dumping - /// - [SuppressMessage("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode", Justification = "Handy helper method that can be used to annotate ReuseableStringBuilder when debugging it, but is not hooked up usually for the sake of perf.")] - [SuppressMessage("Microsoft.Usage", "CA1806:DoNotIgnoreMethodResults", MessageId = "System.String.Format(System.IFormatProvider,System.String,System.Object[])", Justification = "Handy string that can be used to annotate ReuseableStringBuilder when debugging it, but is not hooked up usually.")] - internal static void DumpUnreturned() - { - String.Format(CultureInfo.CurrentUICulture, "{0} Hits of which\n {1} Misses (was on loan)\n {2} Upsizes (needed bigger) \n\n{3} Returns=\n{4} Discards (returned too large)+\n {5} Accepts\n\n{6} estimated bytes saved", s_hits, s_misses, s_upsizes, s_discards + s_accepts, s_discards, s_accepts, s_saved); - - Console.WriteLine("Unreturned string builders were allocated here:"); - foreach (var entry in s_handouts.Values) - { - Console.WriteLine(entry + "\n"); - } - } -#endif - } - } -} diff --git a/src/Shared/StringBuilderCache.cs b/src/Shared/StringBuilderCache.cs deleted file mode 100644 index c5663d2d3e6..00000000000 --- a/src/Shared/StringBuilderCache.cs +++ /dev/null @@ -1,98 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -/*============================================================ -** -** -** Purpose: provide a cached reusable instance of StringBuilder -** per thread it's an optimization that reduces the -** number of instances constructed and collected. -** -** Acquire - is used to get a string builder to use of a -** particular size. It can be called any number of -** times, if a StringBuilder is in the cache then -** it will be returned and the cache emptied. -** subsequent calls will return a new StringBuilder. -** -** A StringBuilder instance is cached in -** Thread Local Storage and so there is one per thread -** -** Release - Place the specified builder in the cache if it is -** not too big. -** The StringBuilder should not be used after it has -** been released. -** Unbalanced Releases are perfectly acceptable. It -** will merely cause the runtime to create a new -** StringBuilder next time Acquire is called. -** -** GetStringAndRelease -** - ToString() the StringBuilder, Release it to the -** cache and return the resulting string -** -===========================================================*/ - -using System; -using System.Text; -#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS -using Microsoft.Build.Eventing; -#endif - -namespace Microsoft.Build.Shared -{ - internal static class StringBuilderCache - { - // The value 360 was chosen in discussion with performance experts as a compromise between using - // as little memory (per thread) as possible and still covering a large part of short-lived - // StringBuilder creations on the startup path of VS designers. - private const int MAX_BUILDER_SIZE = 360; - - [ThreadStatic] - private static StringBuilder t_cachedInstance; - - public static StringBuilder Acquire(int capacity = 16 /*StringBuilder.DefaultCapacity*/) - { - if (capacity <= MAX_BUILDER_SIZE) - { - StringBuilder sb = StringBuilderCache.t_cachedInstance; - if (sb != null) - { - // Avoid StringBuilder block fragmentation by getting a new StringBuilder - // when the requested size is larger than the current capacity - if (capacity <= sb.Capacity) - { - StringBuilderCache.t_cachedInstance = null; - sb.Length = 0; // Equivalent of sb.Clear() that works on .Net 3.5 -#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: sb.GetHashCode(), newCapacity: capacity, oldCapacity: sb.Capacity, type: "sbc-reused"); -#endif - return sb; - } - } - } - - StringBuilder stringBuilder = new StringBuilder(capacity); -#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: stringBuilder.GetHashCode(), newCapacity: capacity, oldCapacity: stringBuilder.Capacity, type: "sbc-new"); -#endif - return stringBuilder; - } - - public static void Release(StringBuilder sb) - { - if (sb.Capacity <= MAX_BUILDER_SIZE) - { - StringBuilderCache.t_cachedInstance = sb; - } -#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: sb.GetHashCode(), returningCapacity: sb.Capacity, returningLength: sb.Length, type: sb.Capacity <= MAX_BUILDER_SIZE ? "sbc-returned" : "sbc-discarded"); -#endif - } - - public static string GetStringAndRelease(StringBuilder sb) - { - string result = sb.ToString(); - Release(sb); - return result; - } - } -} diff --git a/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj b/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj index 3cb8f7f10e4..5ad6f792355 100644 --- a/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj +++ b/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj @@ -57,9 +57,6 @@ ResourceUtilities.cs - - StringBuilderCache.cs - From 8fcec62aef114fd0cad3b2f88e22d29514739561 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Thu, 25 Nov 2021 22:47:41 +0100 Subject: [PATCH 08/26] Minor changes in StringBuilderCache --- src/Framework/StringBuilderCache.cs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/Framework/StringBuilderCache.cs b/src/Framework/StringBuilderCache.cs index a5d334aed41..f984d91c2a4 100644 --- a/src/Framework/StringBuilderCache.cs +++ b/src/Framework/StringBuilderCache.cs @@ -32,6 +32,7 @@ ===========================================================*/ using System; +using System.Diagnostics; using System.Text; #if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS using Microsoft.Build.Eventing; @@ -62,8 +63,8 @@ public static StringBuilder Acquire(int capacity = 16 /*StringBuilder.DefaultCap { StringBuilderCache.t_cachedInstance = null; sb.Length = 0; // Equivalent of sb.Clear() that works on .Net 3.5 -#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: sb.GetHashCode(), newCapacity: capacity, oldCapacity: sb.Capacity, type: "sbc-reused"); +#if DEBUG && !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: sb.GetHashCode(), newCapacity: capacity, oldCapacity: sb.Capacity, type: "sbc-hit"); #endif return sb; } @@ -71,8 +72,8 @@ public static StringBuilder Acquire(int capacity = 16 /*StringBuilder.DefaultCap } StringBuilder stringBuilder = new StringBuilder(capacity); -#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS - MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: stringBuilder.GetHashCode(), newCapacity: capacity, oldCapacity: stringBuilder.Capacity, type: "sbc-new"); +#if DEBUG && !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: stringBuilder.GetHashCode(), newCapacity: capacity, oldCapacity: stringBuilder.Capacity, type: "sbc-miss"); #endif return stringBuilder; } @@ -81,10 +82,14 @@ public static void Release(StringBuilder sb) { if (sb.Capacity <= MAX_BUILDER_SIZE) { + // Assert we are not replacing another string builder. That could happen when Acquire is reentered. + // User of StringBuilderCache has to make sure that calling method call stacks do not also use StringBuilderCache. + Debug.Assert(StringBuilderCache.t_cachedInstance == null, "Unexpected replacing of other StringBuilder."); + StringBuilderCache.t_cachedInstance = sb; } -#if !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS - MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: sb.GetHashCode(), returningCapacity: sb.Capacity, returningLength: sb.Length, type: sb.Capacity <= MAX_BUILDER_SIZE ? "sbc-returned" : "sbc-discarded"); +#if DEBUG && !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS + MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: sb.GetHashCode(), returningCapacity: sb.Capacity, returningLength: sb.Length, type: sb.Capacity <= MAX_BUILDER_SIZE ? "sbc-return" : "sbc-discard"); #endif } From ed763f9b6673e9c4ee75fa5c33afb720e1db8951 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Fri, 26 Nov 2021 15:36:09 +0100 Subject: [PATCH 09/26] Enlarge StrignBuilderCache MAX_BUILDER_SIZE to 512 --- src/Framework/StringBuilderCache.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Framework/StringBuilderCache.cs b/src/Framework/StringBuilderCache.cs index f984d91c2a4..4d4a6076b13 100644 --- a/src/Framework/StringBuilderCache.cs +++ b/src/Framework/StringBuilderCache.cs @@ -42,10 +42,8 @@ namespace Microsoft.Build.Framework { internal static class StringBuilderCache { - // The value 360 was chosen in discussion with performance experts as a compromise between using - // as little memory (per thread) as possible and still covering a large part of short-lived - // StringBuilder creations on the startup path of VS designers. - private const int MAX_BUILDER_SIZE = 360; + // The value 512 was chosen empirically as 99% percentile by captured data. + private const int MAX_BUILDER_SIZE = 512; [ThreadStatic] private static StringBuilder t_cachedInstance; @@ -55,13 +53,13 @@ public static StringBuilder Acquire(int capacity = 16 /*StringBuilder.DefaultCap if (capacity <= MAX_BUILDER_SIZE) { StringBuilder sb = StringBuilderCache.t_cachedInstance; + StringBuilderCache.t_cachedInstance = null; if (sb != null) { // Avoid StringBuilder block fragmentation by getting a new StringBuilder // when the requested size is larger than the current capacity if (capacity <= sb.Capacity) { - StringBuilderCache.t_cachedInstance = null; sb.Length = 0; // Equivalent of sb.Clear() that works on .Net 3.5 #if DEBUG && !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: sb.GetHashCode(), newCapacity: capacity, oldCapacity: sb.Capacity, type: "sbc-hit"); From 6eae7def24f7dec4eb09b414043a8f9b4d659858 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Fri, 26 Nov 2021 15:42:26 +0100 Subject: [PATCH 10/26] ReuseableStringBuilder balance diagnostics --- src/Framework/ReuseableStringBuilder.cs | 28 ++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 945846a4604..29f54c262da 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +#define ASSERT_BALANCE using System; using System.Collections.Generic; @@ -210,6 +211,14 @@ static ReuseableStringBuilderFactory() /// private static StringBuilder s_sharedBuilder; +#if DEBUG + /// + /// Balance between calling Get and Release. + /// Shall be always 0 as Get and 1 at Release. + /// + private static int s_getVsReleaseBalance; +#endif + /// /// Obtains a string builder which may or may not already /// have been used. @@ -217,6 +226,11 @@ static ReuseableStringBuilderFactory() /// internal static StringBuilder Get(int capacity) { +#if DEBUG && ASSERT_BALANCE + int balance = Interlocked.Increment(ref s_getVsReleaseBalance); + Debug.Assert(balance == 1, "Unbalanced Get vs Release. Either forgotten Release or used from multiple threads concurrently."); +#endif + var returned = Interlocked.Exchange(ref s_sharedBuilder, null); if (returned == null) @@ -242,7 +256,9 @@ internal static StringBuilder Get(int capacity) } else { +#if DEBUG MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity: capacity, oldCapacity: returned.Capacity, type: "hit"); +#endif } return returned; @@ -254,6 +270,11 @@ internal static StringBuilder Get(int capacity) /// internal static void Release(ReuseableStringBuilder returning) { +#if DEBUG && ASSERT_BALANCE + int balance = Interlocked.Decrement(ref s_getVsReleaseBalance); + Debug.Assert(balance == 0, "Unbalanced Get vs Release. Either forgotten Release or used from multiple threads concurrently."); +#endif + StringBuilder returningBuilder = returning._borrowedBuilder; int returningLength = returningBuilder.Length; @@ -294,8 +315,9 @@ internal static void Release(ReuseableStringBuilder returning) if (oldSharedBuilder != null) { #if DEBUG - // This can identify in-proper usage from multiple thread or bug in code - forgotten Dispose. - // Look at stack traces of ETW events with reporter string builder hashes. + // This can identify in-proper usage from multiple thread or bug in code - Get was reentered before Release. + // User of ReuseableStringBuilder has to make sure that calling method call stacks do not also use ReuseableStringBuilder. + // Look at stack traces of ETW events which contains reported string builder hashes. MSBuildEventSource.Log.ReusableStringBuilderFactoryReplace(oldHash: oldSharedBuilder.GetHashCode(), newHash: returningBuilder.GetHashCode()); #endif } @@ -315,7 +337,7 @@ private static int SelectBracketedCapacity(int requiredCapacity) } } - // If user wants bigger capacity than maximum respect it. It could be as buffer in p/invoke. + // If user wants bigger capacity than maximum respect it as it could be used as buffer in P/Invoke. return requiredCapacity; } } From 51d30e2cdea616c4e4815a568812889a04da5ede Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Fri, 26 Nov 2021 16:14:42 +0100 Subject: [PATCH 11/26] Using ReusableStringBuilder in ConsoleOutputAligner --- .../Logging/ParallelLogger/ConsoleOutputAligner.cs | 13 +++++-------- src/Framework/ReuseableStringBuilder.cs | 10 ++++++++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/Build/Logging/ParallelLogger/ConsoleOutputAligner.cs b/src/Build/Logging/ParallelLogger/ConsoleOutputAligner.cs index 9f839cfff5f..2fb9b61cfd8 100644 --- a/src/Build/Logging/ParallelLogger/ConsoleOutputAligner.cs +++ b/src/Build/Logging/ParallelLogger/ConsoleOutputAligner.cs @@ -3,8 +3,8 @@ #nullable enable using System; -using System.Diagnostics; using System.Text; +using Microsoft.Build.Framework; using Microsoft.Build.Shared; namespace Microsoft.Build.BackEnd.Logging @@ -20,7 +20,6 @@ internal class ConsoleOutputAligner { internal const int ConsoleTabWidth = 8; - private readonly StringBuilder _reusedStringBuilder = new(1024); private readonly int _bufferWidth; private readonly bool _alignMessages; @@ -52,9 +51,7 @@ public string AlignConsoleOutput(string message, bool prefixAlreadyWritten, int int i = 0; int j = message.IndexOfAny(MSBuildConstants.CrLf); - StringBuilder sb = _reusedStringBuilder; - // prepare reused StringBuilder instance for new use. - sb.Length = 0; + using ReuseableStringBuilder sb = new(); // The string contains new lines, treat each new line as a different string to format and send to the console while (j >= 0) { @@ -72,7 +69,7 @@ public string AlignConsoleOutput(string message, bool prefixAlreadyWritten, int /// /// Append aligned and indented message lines into running . /// - private void AlignAndIndentLineOfMessage(StringBuilder sb, bool prefixAlreadyWritten, int prefixWidth, string message, int start, int count) + private void AlignAndIndentLineOfMessage(ReuseableStringBuilder sb, bool prefixAlreadyWritten, int prefixWidth, string message, int start, int count) { int bufferWidthMinusNewLine = _bufferWidth - 1; @@ -122,7 +119,7 @@ private void AlignAndIndentLineOfMessage(StringBuilder sb, bool prefixAlreadyWri } sb.Append(message, start + index, endIndex - index); - sb.AppendLine(); + sb.Append(Environment.NewLine); index = endIndex; } @@ -136,7 +133,7 @@ private void AlignAndIndentLineOfMessage(StringBuilder sb, bool prefixAlreadyWri } sb.Append(message, start, count); - sb.AppendLine(); + sb.Append(Environment.NewLine); } } } diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 29f54c262da..2cbd823edc9 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -113,6 +113,16 @@ internal ReuseableStringBuilder Append(string value, int startIndex, int count) return this; } + /// + /// Appends a specified number of copies of the string representation of a Unicode character to this instance. + /// + internal ReuseableStringBuilder Append(char value, int repeatCount) + { + LazyPrepare(); + _borrowedBuilder.Append(value, repeatCount); + return this; + } + public ReuseableStringBuilder AppendSeparated(char separator, ICollection strings) { LazyPrepare(); From 312540062b4218aa0eca60a101d7ac4e18ad9633 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Mon, 29 Nov 2021 10:38:11 +0100 Subject: [PATCH 12/26] Dissable balance asserting. --- src/Framework/ReuseableStringBuilder.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 2cbd823edc9..e0f1bc969a4 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -#define ASSERT_BALANCE + +//#define ASSERT_BALANCE using System; using System.Collections.Generic; @@ -221,7 +222,7 @@ static ReuseableStringBuilderFactory() /// private static StringBuilder s_sharedBuilder; -#if DEBUG +#if DEBUG && ASSERT_BALANCE /// /// Balance between calling Get and Release. /// Shall be always 0 as Get and 1 at Release. From 47508ab47305b814ca2e1e77e09db97258b5d50d Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Mon, 29 Nov 2021 13:27:08 +0100 Subject: [PATCH 13/26] Rename ETW event --- src/Framework/MSBuildEventSource.cs | 2 +- src/Framework/ReuseableStringBuilder.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Framework/MSBuildEventSource.cs b/src/Framework/MSBuildEventSource.cs index e93c77a204a..d904b008a11 100644 --- a/src/Framework/MSBuildEventSource.cs +++ b/src/Framework/MSBuildEventSource.cs @@ -503,7 +503,7 @@ public void ReusableStringBuilderFactoryStop(int hash, int returningCapacity, in } [Event(70, Keywords = Keywords.All)] - public void ReusableStringBuilderFactoryReplace(int oldHash, int newHash) + public void ReusableStringBuilderFactoryUnbalanced(int oldHash, int newHash) { WriteEvent(70, oldHash, newHash); } diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index e0f1bc969a4..5dcc137503c 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -329,7 +329,7 @@ internal static void Release(ReuseableStringBuilder returning) // This can identify in-proper usage from multiple thread or bug in code - Get was reentered before Release. // User of ReuseableStringBuilder has to make sure that calling method call stacks do not also use ReuseableStringBuilder. // Look at stack traces of ETW events which contains reported string builder hashes. - MSBuildEventSource.Log.ReusableStringBuilderFactoryReplace(oldHash: oldSharedBuilder.GetHashCode(), newHash: returningBuilder.GetHashCode()); + MSBuildEventSource.Log.ReusableStringBuilderFactoryUnbalanced(oldHash: oldSharedBuilder.GetHashCode(), newHash: returningBuilder.GetHashCode()); #endif } #if DEBUG From a17799168ea7074bae98e5b74283259189372230 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Mon, 29 Nov 2021 13:49:08 +0100 Subject: [PATCH 14/26] Revert: Using ReusableStringBuilder in ConsoleOutputAligner --- .../Logging/ParallelLogger/ConsoleOutputAligner.cs | 13 ++++++++----- src/Framework/ReuseableStringBuilder.cs | 10 ---------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/Build/Logging/ParallelLogger/ConsoleOutputAligner.cs b/src/Build/Logging/ParallelLogger/ConsoleOutputAligner.cs index 2fb9b61cfd8..9f839cfff5f 100644 --- a/src/Build/Logging/ParallelLogger/ConsoleOutputAligner.cs +++ b/src/Build/Logging/ParallelLogger/ConsoleOutputAligner.cs @@ -3,8 +3,8 @@ #nullable enable using System; +using System.Diagnostics; using System.Text; -using Microsoft.Build.Framework; using Microsoft.Build.Shared; namespace Microsoft.Build.BackEnd.Logging @@ -20,6 +20,7 @@ internal class ConsoleOutputAligner { internal const int ConsoleTabWidth = 8; + private readonly StringBuilder _reusedStringBuilder = new(1024); private readonly int _bufferWidth; private readonly bool _alignMessages; @@ -51,7 +52,9 @@ public string AlignConsoleOutput(string message, bool prefixAlreadyWritten, int int i = 0; int j = message.IndexOfAny(MSBuildConstants.CrLf); - using ReuseableStringBuilder sb = new(); + StringBuilder sb = _reusedStringBuilder; + // prepare reused StringBuilder instance for new use. + sb.Length = 0; // The string contains new lines, treat each new line as a different string to format and send to the console while (j >= 0) { @@ -69,7 +72,7 @@ public string AlignConsoleOutput(string message, bool prefixAlreadyWritten, int /// /// Append aligned and indented message lines into running . /// - private void AlignAndIndentLineOfMessage(ReuseableStringBuilder sb, bool prefixAlreadyWritten, int prefixWidth, string message, int start, int count) + private void AlignAndIndentLineOfMessage(StringBuilder sb, bool prefixAlreadyWritten, int prefixWidth, string message, int start, int count) { int bufferWidthMinusNewLine = _bufferWidth - 1; @@ -119,7 +122,7 @@ private void AlignAndIndentLineOfMessage(ReuseableStringBuilder sb, bool prefixA } sb.Append(message, start + index, endIndex - index); - sb.Append(Environment.NewLine); + sb.AppendLine(); index = endIndex; } @@ -133,7 +136,7 @@ private void AlignAndIndentLineOfMessage(ReuseableStringBuilder sb, bool prefixA } sb.Append(message, start, count); - sb.Append(Environment.NewLine); + sb.AppendLine(); } } } diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 5dcc137503c..363588e856f 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -114,16 +114,6 @@ internal ReuseableStringBuilder Append(string value, int startIndex, int count) return this; } - /// - /// Appends a specified number of copies of the string representation of a Unicode character to this instance. - /// - internal ReuseableStringBuilder Append(char value, int repeatCount) - { - LazyPrepare(); - _borrowedBuilder.Append(value, repeatCount); - return this; - } - public ReuseableStringBuilder AppendSeparated(char separator, ICollection strings) { LazyPrepare(); From 8cedfdb4ff17ced1358dd0ecbd58616bae111efc Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Mon, 29 Nov 2021 13:55:25 +0100 Subject: [PATCH 15/26] Change StringBuilderCache max limit --- src/Framework/StringBuilderCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Framework/StringBuilderCache.cs b/src/Framework/StringBuilderCache.cs index 4d4a6076b13..9861b36eb06 100644 --- a/src/Framework/StringBuilderCache.cs +++ b/src/Framework/StringBuilderCache.cs @@ -42,7 +42,7 @@ namespace Microsoft.Build.Framework { internal static class StringBuilderCache { - // The value 512 was chosen empirically as 99% percentile by captured data. + // The value 512 was chosen empirically as 95% percentile private const int MAX_BUILDER_SIZE = 512; [ThreadStatic] From 8470a1f036ce99b21d51f4f53c171363084ed5e3 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Mon, 29 Nov 2021 15:23:34 +0100 Subject: [PATCH 16/26] Disable StringBuilderCache assert ballance --- src/Framework/StringBuilderCache.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Framework/StringBuilderCache.cs b/src/Framework/StringBuilderCache.cs index 9861b36eb06..6ca6563823f 100644 --- a/src/Framework/StringBuilderCache.cs +++ b/src/Framework/StringBuilderCache.cs @@ -31,6 +31,8 @@ ** ===========================================================*/ +//#define ASSERT_BALANCE + using System; using System.Diagnostics; using System.Text; @@ -80,9 +82,11 @@ public static void Release(StringBuilder sb) { if (sb.Capacity <= MAX_BUILDER_SIZE) { +#if ASSERT_BALANCE // Assert we are not replacing another string builder. That could happen when Acquire is reentered. // User of StringBuilderCache has to make sure that calling method call stacks do not also use StringBuilderCache. Debug.Assert(StringBuilderCache.t_cachedInstance == null, "Unexpected replacing of other StringBuilder."); +#endif StringBuilderCache.t_cachedInstance = sb; } From da9c97f820960ab1f8d8e13d65229b011f89a1c4 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Tue, 30 Nov 2021 23:33:33 +0100 Subject: [PATCH 17/26] Enable StringBuilderCache balance asserting --- src/Framework/StringBuilderCache.cs | 5 ----- src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs | 7 +++++-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Framework/StringBuilderCache.cs b/src/Framework/StringBuilderCache.cs index 6ca6563823f..7e255266aa3 100644 --- a/src/Framework/StringBuilderCache.cs +++ b/src/Framework/StringBuilderCache.cs @@ -31,8 +31,6 @@ ** ===========================================================*/ -//#define ASSERT_BALANCE - using System; using System.Diagnostics; using System.Text; @@ -82,12 +80,9 @@ public static void Release(StringBuilder sb) { if (sb.Capacity <= MAX_BUILDER_SIZE) { -#if ASSERT_BALANCE // Assert we are not replacing another string builder. That could happen when Acquire is reentered. // User of StringBuilderCache has to make sure that calling method call stacks do not also use StringBuilderCache. Debug.Assert(StringBuilderCache.t_cachedInstance == null, "Unexpected replacing of other StringBuilder."); -#endif - StringBuilderCache.t_cachedInstance = sb; } #if DEBUG && !CLR2COMPATIBILITY && !MICROSOFT_BUILD_ENGINE_OM_UNITTESTS diff --git a/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs b/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs index 53b19c17a62..3390d2dfd83 100644 --- a/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs +++ b/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs @@ -1087,7 +1087,10 @@ quiet at the engine level. bool logWarning = idealAssemblyRemappingsIdentities.Any(i => i.assemblyName.FullName.Equals(fusionName) && i.reference.GetConflictVictims().Count == 0); StringBuilder logConflict = StringBuilderCache.Acquire(); LogConflict(conflictCandidate, fusionName, logConflict); - StringBuilder logDependencies = logWarning ? logConflict.AppendLine() : StringBuilderCache.Acquire(); + + // If we logging warnings append it into existing StringBuilder, otherwise build details by new StringBuilder. + // Remark: There is no point to use StringBuilderCache.Acquire() here as at this point StringBuilderCache already rent StringBuilder for this thread + StringBuilder logDependencies = logWarning ? logConflict.AppendLine() : new StringBuilder(); // Log the assemblies and primary source items which are related to the conflict which was just logged. Reference victor = dependencyTable.GetReference(conflictCandidate.ConflictVictorName); @@ -1108,7 +1111,7 @@ quiet at the engine level. } else { - details = StringBuilderCache.GetStringAndRelease(logDependencies); + details = logDependencies.ToString(); Log.LogMessage(ChooseReferenceLoggingImportance(conflictCandidate), output); Log.LogMessage(MessageImportance.Low, details); } From 80b3ab1da3a4743e20be499e1a45d0def34454a9 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 1 Dec 2021 14:50:09 +0100 Subject: [PATCH 18/26] Comments in StringBuilderCache --- src/Framework/StringBuilderCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Framework/StringBuilderCache.cs b/src/Framework/StringBuilderCache.cs index 7e255266aa3..abf23d07ff4 100644 --- a/src/Framework/StringBuilderCache.cs +++ b/src/Framework/StringBuilderCache.cs @@ -42,7 +42,7 @@ namespace Microsoft.Build.Framework { internal static class StringBuilderCache { - // The value 512 was chosen empirically as 95% percentile + // The value 512 was chosen empirically as 95% percentile of returning string length. private const int MAX_BUILDER_SIZE = 512; [ThreadStatic] From 7c262060bc91be327ea9e0c4b5840c73a88a1ce2 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 1 Dec 2021 15:22:06 +0100 Subject: [PATCH 19/26] Imrove comments --- src/Framework/ReuseableStringBuilder.cs | 29 +++++++++++++++++-------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 363588e856f..11b7ba68f73 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -1,8 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -//#define ASSERT_BALANCE - using System; using System.Collections.Generic; using System.Diagnostics; @@ -179,14 +177,29 @@ private static class ReuseableStringBuilderFactory /// Made up limit beyond which we won't share the builder /// because we could otherwise hold a huge builder indefinitely. /// This was picked empirically to save at least 95% of allocated data size. - /// This constant has to exactly 2^n (power of 2) where n = 4 ... 32 + /// This constant has to be exactly 2^n (power of 2) where n = 4 ... 32 as GC is optimized to work with such block sizes. + /// Same approach is used in ArrayPool or RecyclableMemoryStream so having same uniform allocation sizes will + /// reduce likelihood of heaps fragmentation. /// /// + /// In order to collect and analyze ETW ReusableStringBuilderFactory events developer could follow these steps: + /// - With compiled as Debug capture events by perfview; example: "perfview collect /NoGui /OnlyProviders=*Microsoft-Build" + /// - Open Events view and filter for ReusableStringBuilderFactory and pick ReusableStringBuilderFactory/Stop + /// - Display columns: returning length, type + /// - Set MaxRet limit to 1_000_000 + /// - Right click and Open View in Excel + /// - Use Excel data analytic tools to extract required data from it. I recommend to use + /// Pivot Table/Chart with + /// filter: type=[return-se,discarder]; + /// rows: returningLength grouped (right click and Group... into sufficient size bins) + /// value: sum of returningLength + /// + /// /// This constant might looks huge, but rather that lowering this constant, /// we shall focus on eliminating of code which requires to create such huge strings. /// private const int MaxBuilderSizeBytes = 2 * 1024 * 1024; // ~1M chars - private const int MaxBuilderSizeCapacity = MaxBuilderSizeBytes / 2; + private const int MaxBuilderSizeCapacity = MaxBuilderSizeBytes / sizeof(char); private static readonly IReadOnlyList s_capacityBrackets; @@ -194,12 +207,10 @@ static ReuseableStringBuilderFactory() { var brackets = new List(); - int bytes = 0x200; // Minimal capacity is 256 (512 bytes) as this was, according to captured traces, mean required capacity + int bytes = 0x200; // Minimal capacity is 256 (512 bytes) as this was, according to captured traces, mean returning capacity while (bytes <= MaxBuilderSizeBytes) { - // Allocation of arrays is optimized in byte[bytes] => bytes = 2^n. - // StringBuilder allocates chars[capacity] and each char is 2 bytes so lets have capacity brackets computed as `bytes/2` - brackets.Add(bytes/2); + brackets.Add(bytes / sizeof(char)); bytes <<= 1; } Debug.Assert((bytes >> 1) == MaxBuilderSizeBytes, "MaxBuilderSizeBytes has to be 2^n (power of 2)"); @@ -271,7 +282,7 @@ internal static StringBuilder Get(int capacity) /// internal static void Release(ReuseableStringBuilder returning) { -#if DEBUG && ASSERT_BALANCE +#if DEBUG && ASSERT_BALANCE // Please define ASSERT_BALANCE if you need to analyze where we have cross thread competing usage of ReuseableStringBuilder int balance = Interlocked.Decrement(ref s_getVsReleaseBalance); Debug.Assert(balance == 0, "Unbalanced Get vs Release. Either forgotten Release or used from multiple threads concurrently."); #endif From 3db14b700f088dbfbb7b26caceeb3481ac961d18 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 1 Dec 2021 15:28:39 +0100 Subject: [PATCH 20/26] Apply suggestions from code review Co-authored-by: Forgind Co-authored-by: Drew Noakes Co-authored-by: Rainer Sigwald --- src/Framework/ReuseableStringBuilder.cs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 11b7ba68f73..d4f267fed05 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -38,9 +38,8 @@ internal sealed class ReuseableStringBuilder : IDisposable /// internal ReuseableStringBuilder(int capacity = 16) // StringBuilder default is 16 { - _capacity = capacity; - // lazy initialization of the builder + _capacity = capacity; } /// @@ -195,8 +194,8 @@ private static class ReuseableStringBuilderFactory /// value: sum of returningLength /// /// - /// This constant might looks huge, but rather that lowering this constant, - /// we shall focus on eliminating of code which requires to create such huge strings. + /// This constant might looks huge, but rather than lowering this constant, + /// we shall focus on eliminating code which requires creating such huge strings. /// private const int MaxBuilderSizeBytes = 2 * 1024 * 1024; // ~1M chars private const int MaxBuilderSizeCapacity = MaxBuilderSizeBytes / sizeof(char); @@ -248,7 +247,7 @@ internal static StringBuilder Get(int capacity) if (returned == null) { // Currently loaned out so return a new one with capacity in given bracket. - // If user wants bigger capacity that maximum capacity respect it. + // If user wants bigger capacity than maximum capacity, respect it. returned = new StringBuilder(SelectBracketedCapacity(capacity)); #if DEBUG MSBuildEventSource.Log.ReusableStringBuilderFactoryStart(hash: returned.GetHashCode(), newCapacity:capacity, oldCapacity:0, type:"miss"); @@ -264,7 +263,7 @@ internal static StringBuilder Get(int capacity) #endif // Let the current StringBuilder be collected and create new with bracketed capacity. This way it allocates only char[newCapacity] // otherwise it would allocate char[new_capacity_of_last_chunk] (in set_Capacity) and char[newCapacity] (in Clear). - returned = new StringBuilder(SelectBracketedCapacity(newCapacity)); + returned = new StringBuilder(newCapacity); } else { @@ -301,7 +300,7 @@ internal static void Release(ReuseableStringBuilder returning) // So the shared builder will be "replaced". if (returningBuilder.Capacity > MaxBuilderSizeCapacity) { - // In order to free memory usage by huge string builder, do not pull this one and let it be collected. + // In order to free memory usage by huge string builder, do not pool this one and let it be collected. #if DEBUG MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: "discard"); #endif @@ -312,11 +311,11 @@ internal static void Release(ReuseableStringBuilder returning) { Debug.Assert(returningBuilder.Capacity > returning._borrowedWithCapacity, "Capacity can only increase"); - // This builder used more that pre-allocated capacity bracket. + // This builder used more than pre-allocated capacity bracket. // Let this builder be collected and put new builder, with reflecting bracket capacity, into the pool. // If we would just return this builder into pool as is, it would allocated new array[capacity] anyway (current implementation of returningBuilder.Clear() does it) // and that could lead to unpredictable amount of LOH allocations and eventual LOH fragmentation. - // Bellow implementation have predictable max Log2(MaxBuilderSizeBytes) string builder array re-allocations during whole process lifetime - unless MaxBuilderSizeCapacity is reached frequently. + // Below implementation has predictable max Log2(MaxBuilderSizeBytes) string builder array re-allocations during whole process lifetime - unless MaxBuilderSizeCapacity is reached frequently. int newCapacity = SelectBracketedCapacity(returningBuilder.Capacity); returningBuilder = new StringBuilder(newCapacity); } @@ -327,7 +326,7 @@ internal static void Release(ReuseableStringBuilder returning) if (oldSharedBuilder != null) { #if DEBUG - // This can identify in-proper usage from multiple thread or bug in code - Get was reentered before Release. + // This can identify improper usage from multiple thread or bug in code - Get was reentered before Release. // User of ReuseableStringBuilder has to make sure that calling method call stacks do not also use ReuseableStringBuilder. // Look at stack traces of ETW events which contains reported string builder hashes. MSBuildEventSource.Log.ReusableStringBuilderFactoryUnbalanced(oldHash: oldSharedBuilder.GetHashCode(), newHash: returningBuilder.GetHashCode()); From 59da9c08bec8c126e391a5de81936fc589e490dc Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 1 Dec 2021 15:51:45 +0100 Subject: [PATCH 21/26] nullable enable --- src/Framework/ReuseableStringBuilder.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 11b7ba68f73..3e04f2d88cf 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -1,9 +1,11 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +#nullable enable using System; using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Text; using System.Threading; using Microsoft.Build.Eventing; @@ -21,7 +23,7 @@ internal sealed class ReuseableStringBuilder : IDisposable /// /// Captured string builder. /// - private StringBuilder _borrowedBuilder; + private StringBuilder? _borrowedBuilder; /// /// Capacity of borrowed string builder at the time of borrowing. @@ -48,7 +50,7 @@ internal sealed class ReuseableStringBuilder : IDisposable /// public int Length { - get { return (_borrowedBuilder == null) ? 0 : _borrowedBuilder.Length; } + get { return _borrowedBuilder?.Length ?? 0; } set { LazyPrepare(); @@ -72,7 +74,7 @@ public override string ToString() /// /// Dispose, indicating you are done with this builder. /// - void IDisposable.Dispose() + public void Dispose() { if (_borrowedBuilder != null) { @@ -153,6 +155,7 @@ internal ReuseableStringBuilder Remove(int startIndex, int length) /// /// Grab a backing builder if necessary. /// + [MemberNotNull(nameof(_borrowedBuilder))] private void LazyPrepare() { if (_borrowedBuilder == null) @@ -221,7 +224,7 @@ static ReuseableStringBuilderFactory() /// /// The shared builder. /// - private static StringBuilder s_sharedBuilder; + private static StringBuilder? s_sharedBuilder; #if DEBUG && ASSERT_BALANCE /// @@ -243,7 +246,7 @@ internal static StringBuilder Get(int capacity) Debug.Assert(balance == 1, "Unbalanced Get vs Release. Either forgotten Release or used from multiple threads concurrently."); #endif - var returned = Interlocked.Exchange(ref s_sharedBuilder, null); + StringBuilder? returned = Interlocked.Exchange(ref s_sharedBuilder, null); if (returned == null) { @@ -287,7 +290,7 @@ internal static void Release(ReuseableStringBuilder returning) Debug.Assert(balance == 0, "Unbalanced Get vs Release. Either forgotten Release or used from multiple threads concurrently."); #endif - StringBuilder returningBuilder = returning._borrowedBuilder; + StringBuilder returningBuilder = returning._borrowedBuilder!; int returningLength = returningBuilder.Length; // It's possible for someone to cause the builder to From 43d7df90b50e7b876c368b87a227012854ea5d71 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 1 Dec 2021 16:14:40 +0100 Subject: [PATCH 22/26] Make sure borrowed string builder can not be used after Release --- src/Framework/ReuseableStringBuilder.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 2695b37c949..c7e43ef0060 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -288,6 +288,7 @@ internal static void Release(ReuseableStringBuilder returning) int balance = Interlocked.Decrement(ref s_getVsReleaseBalance); Debug.Assert(balance == 0, "Unbalanced Get vs Release. Either forgotten Release or used from multiple threads concurrently."); #endif + FrameworkErrorUtilities.VerifyThrowInternalNull(returning._borrowedBuilder, nameof(returning._borrowedBuilder) + " can not be null."); StringBuilder returningBuilder = returning._borrowedBuilder!; int returningLength = returningBuilder.Length; @@ -339,6 +340,9 @@ internal static void Release(ReuseableStringBuilder returning) MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: returning._borrowedBuilder != returningBuilder ? "return-new" : "return"); #endif } + + // Ensure ReuseableStringBuilder can no longer use _borrowedBuilder + returning._borrowedBuilder = null; } private static int SelectBracketedCapacity(int requiredCapacity) From 4a3341791f450c53a40365b751071cfacc06ceb1 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 1 Dec 2021 16:56:08 +0100 Subject: [PATCH 23/26] Modified SelectBracketedCapacity by Drew comment. --- src/Framework/ReuseableStringBuilder.cs | 44 +++++++++++-------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index c7e43ef0060..cd70959bfbf 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -203,23 +203,6 @@ private static class ReuseableStringBuilderFactory private const int MaxBuilderSizeBytes = 2 * 1024 * 1024; // ~1M chars private const int MaxBuilderSizeCapacity = MaxBuilderSizeBytes / sizeof(char); - private static readonly IReadOnlyList s_capacityBrackets; - - static ReuseableStringBuilderFactory() - { - var brackets = new List(); - - int bytes = 0x200; // Minimal capacity is 256 (512 bytes) as this was, according to captured traces, mean returning capacity - while (bytes <= MaxBuilderSizeBytes) - { - brackets.Add(bytes / sizeof(char)); - bytes <<= 1; - } - Debug.Assert((bytes >> 1) == MaxBuilderSizeBytes, "MaxBuilderSizeBytes has to be 2^n (power of 2)"); - - s_capacityBrackets = brackets; - } - /// /// The shared builder. /// @@ -347,16 +330,27 @@ internal static void Release(ReuseableStringBuilder returning) private static int SelectBracketedCapacity(int requiredCapacity) { - foreach (int bracket in s_capacityBrackets) - { - if (requiredCapacity <= bracket) - { - return bracket; - } - } + const int minimumCapacity = 0x100; // 256 characters, 512 bytes + + if (requiredCapacity <= minimumCapacity) + return minimumCapacity; // If user wants bigger capacity than maximum respect it as it could be used as buffer in P/Invoke. - return requiredCapacity; + if (requiredCapacity >= MaxBuilderSizeCapacity) + return requiredCapacity; + + // Find next power of two http://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2 + int v = requiredCapacity; + + v--; + v |= v >> 1; + v |= v >> 2; + v |= v >> 4; + v |= v >> 8; + v |= v >> 16; + v++; + + return v; } } } From 4fd193517443d34e551126172992b77c381b5769 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 1 Dec 2021 18:23:14 +0100 Subject: [PATCH 24/26] Commenting ETW --- src/Framework/MSBuildEventSource.cs | 11 +++++++++++ src/Framework/ReuseableStringBuilder.cs | 2 -- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Framework/MSBuildEventSource.cs b/src/Framework/MSBuildEventSource.cs index d904b008a11..f8587c129c4 100644 --- a/src/Framework/MSBuildEventSource.cs +++ b/src/Framework/MSBuildEventSource.cs @@ -490,18 +490,29 @@ public void CachedSdkResolverServiceResolveSdkStop(string sdkName, string soluti WriteEvent(67, sdkName, solutionPath, projectPath, success); } + /// + /// This events are quite frequent so they are collected by Debug binaries only. + /// [Event(68, Keywords = Keywords.All)] public void ReusableStringBuilderFactoryStart(int hash, int newCapacity, int oldCapacity, string type) { WriteEvent(68, hash, newCapacity, oldCapacity, type); } + /// + /// This events are quite frequent so they are collected by Debug binaries only. + /// [Event(69, Keywords = Keywords.All)] public void ReusableStringBuilderFactoryStop(int hash, int returningCapacity, int returningLength, string type) { WriteEvent(69, hash, returningCapacity, returningLength, type); } + /// + /// As oppose to other ReusableStringBuilderFactory events this one is expected to happens very un-frequently + /// and if it is seen more than 100x per build it might indicates wrong usage patterns resulting into degrading + /// efficiency of ReusableStringBuilderFactory. Hence it is collected in release build as well. + /// [Event(70, Keywords = Keywords.All)] public void ReusableStringBuilderFactoryUnbalanced(int oldHash, int newHash) { diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index cd70959bfbf..72b7ea47073 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -312,12 +312,10 @@ internal static void Release(ReuseableStringBuilder returning) var oldSharedBuilder = Interlocked.Exchange(ref s_sharedBuilder, returningBuilder); if (oldSharedBuilder != null) { -#if DEBUG // This can identify improper usage from multiple thread or bug in code - Get was reentered before Release. // User of ReuseableStringBuilder has to make sure that calling method call stacks do not also use ReuseableStringBuilder. // Look at stack traces of ETW events which contains reported string builder hashes. MSBuildEventSource.Log.ReusableStringBuilderFactoryUnbalanced(oldHash: oldSharedBuilder.GetHashCode(), newHash: returningBuilder.GetHashCode()); -#endif } #if DEBUG MSBuildEventSource.Log.ReusableStringBuilderFactoryStop(hash: returningBuilder.GetHashCode(), returningCapacity: returningBuilder.Capacity, returningLength: returningLength, type: returning._borrowedBuilder != returningBuilder ? "return-new" : "return"); From 58ac763f440379c1370f18c260cc2d77b8c9d5a4 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 1 Dec 2021 18:30:34 +0100 Subject: [PATCH 25/26] Comment typo --- src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs b/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs index 3390d2dfd83..5cb27a7ab28 100644 --- a/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs +++ b/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs @@ -1088,7 +1088,7 @@ quiet at the engine level. StringBuilder logConflict = StringBuilderCache.Acquire(); LogConflict(conflictCandidate, fusionName, logConflict); - // If we logging warnings append it into existing StringBuilder, otherwise build details by new StringBuilder. + // If we are logging warnings append it into existing StringBuilder, otherwise build details by new StringBuilder. // Remark: There is no point to use StringBuilderCache.Acquire() here as at this point StringBuilderCache already rent StringBuilder for this thread StringBuilder logDependencies = logWarning ? logConflict.AppendLine() : new StringBuilder(); From 103abfd44bcfde86c7d237d5d1ef1949c66d9114 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Thu, 2 Dec 2021 10:41:58 +0100 Subject: [PATCH 26/26] VerifyThrowInternalNull meesage --- src/Framework/ReuseableStringBuilder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Framework/ReuseableStringBuilder.cs b/src/Framework/ReuseableStringBuilder.cs index 72b7ea47073..18457bee6f6 100644 --- a/src/Framework/ReuseableStringBuilder.cs +++ b/src/Framework/ReuseableStringBuilder.cs @@ -271,7 +271,7 @@ internal static void Release(ReuseableStringBuilder returning) int balance = Interlocked.Decrement(ref s_getVsReleaseBalance); Debug.Assert(balance == 0, "Unbalanced Get vs Release. Either forgotten Release or used from multiple threads concurrently."); #endif - FrameworkErrorUtilities.VerifyThrowInternalNull(returning._borrowedBuilder, nameof(returning._borrowedBuilder) + " can not be null."); + FrameworkErrorUtilities.VerifyThrowInternalNull(returning._borrowedBuilder, nameof(returning._borrowedBuilder)); StringBuilder returningBuilder = returning._borrowedBuilder!; int returningLength = returningBuilder.Length;