From 8b59434140309d2bf0415f1481951c93c165e794 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 24 Aug 2020 15:10:12 -0700 Subject: [PATCH 1/9] Passes 0 and 1 Optimizations --- src/Build/Evaluation/Evaluator.cs | 130 +++++++----------- src/Build/Evaluation/ItemSpec.cs | 4 +- src/Build/Microsoft.Build.csproj | 3 - src/Build/Utilities/FileSpecMatchTester.cs | 3 +- .../EscapingStringExtensions.cs | 34 ----- src/Shared/EscapingUtilities.cs | 43 +++--- src/Shared/FileUtilities.cs | 14 ++ .../Microsoft.Build.Utilities.csproj | 3 - 8 files changed, 85 insertions(+), 149 deletions(-) delete mode 100644 src/Shared/EscapingStringExtensions/EscapingStringExtensions.cs diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index e27ee2eb1fa..a95105e72b3 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -616,7 +616,7 @@ private void Evaluate() _logProjectImportedEvents = Traits.Instance.EscapeHatches.LogProjectImports; - ICollection

globalProperties; + int globalPropertiesCount; using (_evaluationProfiler.TrackPass(EvaluationPass.InitialProperties)) { @@ -626,7 +626,7 @@ private void Evaluate() AddBuiltInProperties(); AddEnvironmentProperties(); AddToolsetProperties(); - globalProperties = AddGlobalProperties(); + globalPropertiesCount = AddGlobalProperties(); if (_interactive) { @@ -652,7 +652,7 @@ private void Evaluate() List initialTargets = new List(_initialTargetsList.Count); foreach (var initialTarget in _initialTargetsList) { - initialTargets.Add(EscapingUtilities.UnescapeAll(initialTarget.Trim())); + initialTargets.Add(EscapingUtilities.UnescapeAll(initialTarget, true)); } _data.InitialTargets = initialTargets; @@ -789,7 +789,7 @@ private void Evaluate() string line = new string('#', 100) + "\n"; - string output = String.Format(CultureInfo.CurrentUICulture, "###: MSBUILD: Evaluating or reevaluating project {0} with {1} global properties and {2} tools version, child count {3}, CurrentSolutionConfigurationContents hash {4} other properties:\n{5}", _projectRootElement.FullPath, globalProperties.Count, _data.Toolset.ToolsVersion, _projectRootElement.Count, hash, propertyDump); + string output = String.Format(CultureInfo.CurrentUICulture, "###: MSBUILD: Evaluating or reevaluating project {0} with {1} global properties and {2} tools version, child count {3}, CurrentSolutionConfigurationContents hash {4} other properties:\n{5}", _projectRootElement.FullPath, globalPropertiesCount, _data.Toolset.ToolsVersion, _projectRootElement.Count, hash, propertyDump); Trace.WriteLine(line + output + line); } @@ -964,7 +964,7 @@ private void UpdateDefaultTargets(ProjectRootElement currentProjectOrImport) for (int i = 0; i < temp.Count; i++) { - string target = EscapingUtilities.UnescapeAll(temp[i].Trim()); + string target = EscapingUtilities.UnescapeAll(temp[i], true); if (target.Length > 0) { _data.DefaultTargets ??= new List(temp.Count); @@ -1130,106 +1130,88 @@ private void AddBeforeAndAfterTargetMappings(ProjectTargetElement targetElement, ///

/// Set the built-in properties, most of which are read-only /// - private ICollection

AddBuiltInProperties() + private void AddBuiltInProperties() { string startupDirectory = BuildParameters.StartupDirectory; - List

builtInProperties = new List

(19); - - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.toolsVersion, _data.Toolset.ToolsVersion)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.toolsPath, _data.Toolset.ToolsPath)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.binPath, _data.Toolset.ToolsPath)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.startupDirectory, startupDirectory)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.buildNodeCount, _maxNodeCount.ToString(CultureInfo.CurrentCulture))); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.programFiles32, FrameworkLocationHelper.programFiles32)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.assemblyVersion, Constants.AssemblyVersion)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.version, MSBuildAssemblyFileVersion.Instance.MajorMinorBuild)); + SetBuiltInProperty(ReservedPropertyNames.toolsVersion, _data.Toolset.ToolsVersion); + SetBuiltInProperty(ReservedPropertyNames.toolsPath, _data.Toolset.ToolsPath); + SetBuiltInProperty(ReservedPropertyNames.binPath, _data.Toolset.ToolsPath); + SetBuiltInProperty(ReservedPropertyNames.startupDirectory, startupDirectory); + SetBuiltInProperty(ReservedPropertyNames.buildNodeCount, _maxNodeCount.ToString(CultureInfo.CurrentCulture)); + SetBuiltInProperty(ReservedPropertyNames.programFiles32, FrameworkLocationHelper.programFiles32); + SetBuiltInProperty(ReservedPropertyNames.assemblyVersion, Constants.AssemblyVersion); + SetBuiltInProperty(ReservedPropertyNames.version, MSBuildAssemblyFileVersion.Instance.MajorMinorBuild); // Fake OS env variables when not on Windows if (!NativeMethodsShared.IsWindows) { - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.osName, NativeMethodsShared.OSName)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.frameworkToolsRoot, NativeMethodsShared.FrameworkBasePath)); + SetBuiltInProperty(ReservedPropertyNames.osName, NativeMethodsShared.OSName); + SetBuiltInProperty(ReservedPropertyNames.frameworkToolsRoot, NativeMethodsShared.FrameworkBasePath); } #if RUNTIME_TYPE_NETCORE - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.msbuildRuntimeType, "Core")); + SetBuiltInProperty(ReservedPropertyNames.msbuildRuntimeType, "Core"); #elif MONO - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.msbuildRuntimeType, - NativeMethodsShared.IsMono ? "Mono" : "Full")); + SetBuiltInProperty(ReservedPropertyNames.msbuildRuntimeType, + NativeMethodsShared.IsMono ? "Mono" : "Full"); #else - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.msbuildRuntimeType, "Full")); + SetBuiltInProperty(ReservedPropertyNames.msbuildRuntimeType, "Full"); #endif if (String.IsNullOrEmpty(_projectRootElement.FullPath)) { - // If this is an un-saved project, this is as far as we can go - if (String.IsNullOrEmpty(_projectRootElement.DirectoryPath)) - { - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.projectDirectory, startupDirectory)); - } - else - { + SetBuiltInProperty(ReservedPropertyNames.projectDirectory, String.IsNullOrEmpty(_projectRootElement.DirectoryPath) ? + // If this is an un-saved project, this is as far as we can go + startupDirectory : // Solution files based on the old OM end up here. But they do have a location, which is where the solution was loaded from. // We need to set this here otherwise we can't locate any projects the solution refers to. - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.projectDirectory, _projectRootElement.DirectoryPath)); - } + _projectRootElement.DirectoryPath); } else { // Add the MSBuildProjectXXXXX properties, but not the MSBuildFileXXXX ones. Those // vary according to the file they're evaluated in, so they have to be dealt with // specially in the Expander. - string projectFile = EscapingUtilities.Escape(Path.GetFileName(_projectRootElement.FullPath)); string projectFileWithoutExtension = EscapingUtilities.Escape(Path.GetFileNameWithoutExtension(_projectRootElement.FullPath)); string projectExtension = EscapingUtilities.Escape(Path.GetExtension(_projectRootElement.FullPath)); - string projectFullPath = EscapingUtilities.Escape(_projectRootElement.FullPath); + string projectFile = projectFileWithoutExtension + projectExtension; string projectDirectory = EscapingUtilities.Escape(_projectRootElement.DirectoryPath); + string projectFullPath = Path.Combine(projectDirectory, projectFile); int rootLength = Path.GetPathRoot(projectDirectory).Length; string projectDirectoryNoRoot = projectDirectory.Substring(rootLength); - projectDirectoryNoRoot = FileUtilities.EnsureNoTrailingSlash(projectDirectoryNoRoot); - projectDirectoryNoRoot = EscapingUtilities.Escape(FileUtilities.EnsureNoLeadingSlash(projectDirectoryNoRoot)); + projectDirectoryNoRoot = FileUtilities.EnsureNoLeadingOrTrailingSlash(projectDirectoryNoRoot); // ReservedPropertyNames.projectDefaultTargets is already set - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.projectFile, projectFile)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.projectName, projectFileWithoutExtension)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.projectExtension, projectExtension)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.projectFullPath, projectFullPath)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.projectDirectory, projectDirectory)); - builtInProperties.Add(SetBuiltInProperty(ReservedPropertyNames.projectDirectoryNoRoot, projectDirectoryNoRoot)); + SetBuiltInProperty(ReservedPropertyNames.projectFile, projectFile); + SetBuiltInProperty(ReservedPropertyNames.projectName, projectFileWithoutExtension); + SetBuiltInProperty(ReservedPropertyNames.projectExtension, projectExtension); + SetBuiltInProperty(ReservedPropertyNames.projectFullPath, projectFullPath); + SetBuiltInProperty(ReservedPropertyNames.projectDirectory, projectDirectory); + SetBuiltInProperty(ReservedPropertyNames.projectDirectoryNoRoot, projectDirectoryNoRoot); } - - return builtInProperties; } ///

/// Pull in all the environment into our property bag /// - private ICollection

AddEnvironmentProperties() + private void AddEnvironmentProperties() { - List

environmentPropertiesList = new List

(_environmentProperties.Count); - foreach (ProjectPropertyInstance environmentProperty in _environmentProperties) { - P property = _data.SetProperty(environmentProperty.Name, ((IProperty)environmentProperty).EvaluatedValueEscaped, isGlobalProperty: false, mayBeReserved: false, isEnvironmentVariable: true); - environmentPropertiesList.Add(property); + _data.SetProperty(environmentProperty.Name, ((IProperty)environmentProperty).EvaluatedValueEscaped, isGlobalProperty: false, mayBeReserved: false, isEnvironmentVariable: true); } - - return environmentPropertiesList; } ///

/// Put all the toolset's properties into our property bag /// - private ICollection

AddToolsetProperties() + private void AddToolsetProperties() { - List

toolsetProperties = new List

(_data.Toolset.Properties.Count); - foreach (ProjectPropertyInstance toolsetProperty in _data.Toolset.Properties.Values) { - P property = _data.SetProperty(toolsetProperty.Name, ((IProperty)toolsetProperty).EvaluatedValueEscaped, false /* NOT global property */, false /* may NOT be a reserved name */); - toolsetProperties.Add(property); + _data.SetProperty(toolsetProperty.Name, ((IProperty)toolsetProperty).EvaluatedValueEscaped, false /* NOT global property */, false /* may NOT be a reserved name */); } if (_data.SubToolsetVersion == null) @@ -1238,55 +1220,45 @@ private ICollection

AddToolsetProperties() // is most likely not a subtoolset now, we need to add VisualStudioVersion if its not already a property. if (!_data.Properties.Contains(Constants.VisualStudioVersionPropertyName)) { - P subToolsetVersionProperty = _data.SetProperty(Constants.VisualStudioVersionPropertyName, MSBuildConstants.CurrentVisualStudioVersion, false /* NOT global property */, false /* may NOT be a reserved name */); - toolsetProperties.Add(subToolsetVersionProperty); + _data.SetProperty(Constants.VisualStudioVersionPropertyName, MSBuildConstants.CurrentVisualStudioVersion, false /* NOT global property */, false /* may NOT be a reserved name */); } } else { - // Make the subtoolset version itself available as a property -- but only if it's not already set. // Because some people may be depending on this value even if there isn't a matching sub-toolset, // set the property even if there is no matching sub-toolset. if (!_data.Properties.Contains(Constants.SubToolsetVersionPropertyName)) { - P subToolsetVersionProperty = _data.SetProperty(Constants.SubToolsetVersionPropertyName, _data.SubToolsetVersion, false /* NOT global property */, false /* may NOT be a reserved name */); - toolsetProperties.Add(subToolsetVersionProperty); + _data.SetProperty(Constants.SubToolsetVersionPropertyName, _data.SubToolsetVersion, false /* NOT global property */, false /* may NOT be a reserved name */); } - SubToolset subToolset; - if (_data.Toolset.SubToolsets.TryGetValue(_data.SubToolsetVersion, out subToolset)) + if (_data.Toolset.SubToolsets.TryGetValue(_data.SubToolsetVersion, out SubToolset subToolset)) { foreach (ProjectPropertyInstance subToolsetProperty in subToolset.Properties.Values) { - P property = _data.SetProperty(subToolsetProperty.Name, ((IProperty)subToolsetProperty).EvaluatedValueEscaped, false /* NOT global property */, false /* may NOT be a reserved name */); - toolsetProperties.Add(property); + _data.SetProperty(subToolsetProperty.Name, ((IProperty)subToolsetProperty).EvaluatedValueEscaped, false /* NOT global property */, false /* may NOT be a reserved name */); } } } - return toolsetProperties; } ///

/// Put all the global properties into our property bag /// - private ICollection

AddGlobalProperties() + private int AddGlobalProperties() { - if (_data.GlobalPropertiesDictionary == null) + int count = 0; + if (_data.GlobalPropertiesDictionary != null) { - return Array.Empty

(); - } - - List

globalProperties = new List

(_data.GlobalPropertiesDictionary.Count); - - foreach (ProjectPropertyInstance globalProperty in _data.GlobalPropertiesDictionary) - { - P property = _data.SetProperty(globalProperty.Name, ((IProperty)globalProperty).EvaluatedValueEscaped, true /* IS global property */, false /* may NOT be a reserved name */); - globalProperties.Add(property); + foreach (ProjectPropertyInstance globalProperty in _data.GlobalPropertiesDictionary) + { + _data.SetProperty(globalProperty.Name, ((IProperty)globalProperty).EvaluatedValueEscaped, true /* IS global property */, false /* may NOT be a reserved name */); + count++; + } } - - return globalProperties; + return count; } ///

@@ -2565,7 +2537,7 @@ private void SetAllProjectsProperty() { P oldValue = _data.GetProperty(Constants.MSBuildAllProjectsPropertyName); string streamImports = string.Join(";", _streamImports.ToArray()); - P newValue = _data.SetProperty( + _data.SetProperty( Constants.MSBuildAllProjectsPropertyName, oldValue == null ? $"{_lastModifiedProject.FullPath}{streamImports}" diff --git a/src/Build/Evaluation/ItemSpec.cs b/src/Build/Evaluation/ItemSpec.cs index 165407a7f02..631925c067a 100644 --- a/src/Build/Evaluation/ItemSpec.cs +++ b/src/Build/Evaluation/ItemSpec.cs @@ -4,11 +4,9 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Runtime.InteropServices.WindowsRuntime; using Microsoft.Build.Globbing; using Microsoft.Build.Internal; using Microsoft.Build.Shared; -using Microsoft.Build.Shared.EscapingStringExtensions; namespace Microsoft.Build.Evaluation { @@ -475,7 +473,7 @@ public virtual IMSBuildGlob ToMSBuildGlob() protected virtual IMSBuildGlob CreateMsBuildGlob() { - return MSBuildGlob.Parse(ProjectDirectory, TextFragment.Unescape()); + return MSBuildGlob.Parse(ProjectDirectory, EscapingUtilities.UnescapeAll(TextFragment)); } private FileSpecMatcherTester CreateFileSpecMatcher() diff --git a/src/Build/Microsoft.Build.csproj b/src/Build/Microsoft.Build.csproj index 43a84039b2e..e7e84b1fdad 100644 --- a/src/Build/Microsoft.Build.csproj +++ b/src/Build/Microsoft.Build.csproj @@ -696,9 +696,6 @@ SharedUtilities\ExceptionHandling.cs - - SharedUtilities\EscapingStringExtensions\EscapingStringExtensions.cs - SharedUtilities\FileMatcher.cs true diff --git a/src/Build/Utilities/FileSpecMatchTester.cs b/src/Build/Utilities/FileSpecMatchTester.cs index 24d895b8e67..af7c6dde302 100644 --- a/src/Build/Utilities/FileSpecMatchTester.cs +++ b/src/Build/Utilities/FileSpecMatchTester.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using Microsoft.Build.Shared; -using Microsoft.Build.Shared.EscapingStringExtensions; using System; using System.Diagnostics; using System.IO; @@ -27,7 +26,7 @@ private FileSpecMatcherTester(string currentDirectory, string unescapedFileSpec, public static FileSpecMatcherTester Parse(string currentDirectory, string fileSpec) { - string unescapedFileSpec = fileSpec.Unescape(); + string unescapedFileSpec = EscapingUtilities.UnescapeAll(fileSpec); Regex regex = EngineFileUtilities.FilespecHasWildcards(fileSpec) ? CreateRegex(unescapedFileSpec, currentDirectory) : null; return new FileSpecMatcherTester(currentDirectory, unescapedFileSpec, regex); diff --git a/src/Shared/EscapingStringExtensions/EscapingStringExtensions.cs b/src/Shared/EscapingStringExtensions/EscapingStringExtensions.cs deleted file mode 100644 index ae749f47692..00000000000 --- a/src/Shared/EscapingStringExtensions/EscapingStringExtensions.cs +++ /dev/null @@ -1,34 +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 Microsoft.Build.Shared; - -namespace Microsoft.Build.Shared.EscapingStringExtensions -{ - internal static class EscapingStringExtensions - { - internal static string Unescape(this string escapedString) - { - return EscapingUtilities.UnescapeAll(escapedString); - } - - internal static string Unescape - ( - this string escapedString, - out bool escapingWasNecessary - ) - { - return EscapingUtilities.UnescapeAll(escapedString, out escapingWasNecessary); - } - - internal static string Escape(this string unescapedString) - { - return EscapingUtilities.Escape(unescapedString); - } - - internal static bool ContainsEscapedWildcards(this string escapedString) - { - return EscapingUtilities.ContainsEscapedWildcards(escapedString); - } - } -} \ No newline at end of file diff --git a/src/Shared/EscapingUtilities.cs b/src/Shared/EscapingUtilities.cs index 965175bdbed..ae7c5b67a1e 100644 --- a/src/Shared/EscapingUtilities.cs +++ b/src/Shared/EscapingUtilities.cs @@ -24,20 +24,6 @@ static internal class EscapingUtilities /// private static Dictionary s_unescapedToEscapedStrings = new Dictionary(StringComparer.Ordinal); - /// - /// Replaces all instances of %XX in the input string with the character represented - /// by the hexadecimal number XX. - /// - /// The string to unescape. - /// unescaped string - internal static string UnescapeAll - ( - string escapedString - ) - { - return UnescapeAll(escapedString, out bool _); - } - private static bool IsHexDigit(char character) { return ((character >= '0') && (character <= '9')) @@ -50,16 +36,9 @@ private static bool IsHexDigit(char character) /// by the hexadecimal number XX. /// /// The string to unescape. - /// Whether any replacements were made. /// unescaped string - internal static string UnescapeAll - ( - string escapedString, - out bool escapingWasNecessary - ) + internal static string UnescapeAll(string escapedString, bool trim = false) { - escapingWasNecessary = false; - // If the string doesn't contain anything, then by definition it doesn't // need unescaping. if (String.IsNullOrEmpty(escapedString)) @@ -79,6 +58,22 @@ internal static string UnescapeAll StringBuilder unescapedString = StringBuilderCache.Acquire(escapedString.Length); int currentPosition = 0; + int escapedStringLength = escapedString.Length; + if (trim) + { + while (currentPosition < escapedString.Length && Char.IsWhiteSpace(escapedString[currentPosition])) + { + currentPosition++; + } + if (currentPosition == escapedString.Length) + { + return String.Empty; + } + while (Char.IsWhiteSpace(escapedString[escapedStringLength])) + { + escapedStringLength--; + } + } // Loop until there are no more percent signs in the input string. while (indexOfPercent != -1) @@ -106,8 +101,6 @@ internal static string UnescapeAll // Advance the current pointer to reflect the fact that the destination string // is up to date with everything up to and including this escape code we just found. currentPosition = indexOfPercent + 3; - - escapingWasNecessary = true; } // Find the next percent sign. @@ -116,7 +109,7 @@ internal static string UnescapeAll // Okay, there are no more percent signs in the input string, so just copy the remaining // characters into the destination. - unescapedString.Append(escapedString, currentPosition, escapedString.Length - currentPosition); + unescapedString.Append(escapedString, currentPosition, escapedStringLength - currentPosition); return StringBuilderCache.GetStringAndRelease(unescapedString); } diff --git a/src/Shared/FileUtilities.cs b/src/Shared/FileUtilities.cs index ca576a58036..fecfa69a3b7 100644 --- a/src/Shared/FileUtilities.cs +++ b/src/Shared/FileUtilities.cs @@ -223,6 +223,20 @@ internal static string EnsureNoTrailingSlash(string path) return path; } + /// + /// Ensures the path does not have a leading or trailing slash. + /// + internal static string EnsureNoLeadingOrTrailingSlash(string path) + { + if (String.IsNullOrEmpty(path)) + { + return path; + } + int start = IsSlash(path[0]) ? 1 : 0; + int end = IsSlash(path[path.Length - 1]) ? 1 : 0; + return path.Substring(start, path.Length - start - end); + } + /// /// Indicates if the given file-spec ends with a slash. /// diff --git a/src/Utilities/Microsoft.Build.Utilities.csproj b/src/Utilities/Microsoft.Build.Utilities.csproj index e8b84435ca2..840d2bcc37e 100644 --- a/src/Utilities/Microsoft.Build.Utilities.csproj +++ b/src/Utilities/Microsoft.Build.Utilities.csproj @@ -88,9 +88,6 @@ Shared\ExceptionHandling.cs - - Shared\EscapingStringExtensions\EscapingStringExtensions.cs - Shared\FileUtilities.cs From 07a77e0f39e7da775995f5c68d9a1b0a72661fcb Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 24 Aug 2020 15:33:45 -0700 Subject: [PATCH 2/9] Optimize pass 2 This is a hot path, and someone added an optimization to verify that expresions are nonempty before expanding them. This check was duplicated, however: every time it is called for these two methods, it already could not be empty. This removes the duplication. --- src/Build/Evaluation/Expander.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 4322d3fcaa2..fa5a75cfa5a 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -738,11 +738,6 @@ internal static string ExpandMetadataLeaveEscaped(string expression, IMetadataTa return expression; } - if (expression.Length == 0) - { - return expression; - } - ErrorUtilities.VerifyThrow(metadata != null, "Cannot expand metadata without providing metadata"); // PERF NOTE: Regex matching is expensive, so if the string doesn't contain any item metadata references, just bail @@ -1938,7 +1933,7 @@ private static class ItemExpander internal static string ExpandItemVectorsIntoString(Expander expander, string expression, IItemProvider items, ExpanderOptions options, IElementLocation elementLocation) where T : class, IItem { - if (((options & ExpanderOptions.ExpandItems) == 0) || (expression.Length == 0)) + if ((options & ExpanderOptions.ExpandItems) == 0) { return expression; } From 1e1939cf36f480aaa87180943236175624ee830a Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 24 Aug 2020 17:37:23 -0700 Subject: [PATCH 3/9] cleanup compile errors Also inlined GetItemList and one of the private overloads of EvaluateCondition --- src/Build/Definition/ProjectProperty.cs | 10 +++--- src/Build/Evaluation/LazyItemEvaluator.cs | 35 ++++--------------- .../ProjectPropertyLink.cs | 2 +- .../LinkedObjectFactory.cs | 2 +- src/Build/Utilities/EngineFileUtilities.cs | 2 +- src/Shared/EscapingUtilities.cs | 1 + 6 files changed, 15 insertions(+), 37 deletions(-) diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index a8e6b32e4a3..df1a9b1629e 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -49,7 +49,7 @@ internal ProjectProperty(Project project, string evaluatedValueEscaped) _evaluatedValueEscaped = evaluatedValueEscaped; } - internal virtual string EvaluatedValueEscapedIntenral => _evaluatedValueEscaped; + internal virtual string EvaluatedValueEscapedInternal => _evaluatedValueEscaped; /// /// Name of the property. @@ -79,7 +79,7 @@ public string EvaluatedValue { [DebuggerStepThrough] get - { return EscapingUtilities.UnescapeAll(EvaluatedValueEscapedIntenral); } + { return EscapingUtilities.UnescapeAll(EvaluatedValueEscapedInternal); } } /// @@ -94,7 +94,7 @@ public string EvaluatedValue string IProperty.EvaluatedValueEscaped { [DebuggerStepThrough] - get => EvaluatedValueEscapedIntenral; + get => EvaluatedValueEscapedInternal; } /// @@ -201,7 +201,7 @@ string IKeyed.Key string IValued.EscapedValue { [DebuggerStepThrough] - get => EvaluatedValueEscapedIntenral; + get => EvaluatedValueEscapedInternal; } #region IEquatable Members @@ -225,7 +225,7 @@ bool IEquatable.Equals(ProjectProperty other) return _project == other._project && Xml == other.Xml && - EvaluatedValueEscapedIntenral == other.EvaluatedValueEscapedIntenral && + EvaluatedValueEscapedInternal == other.EvaluatedValueEscapedInternal && Name == other.Name; } diff --git a/src/Build/Evaluation/LazyItemEvaluator.cs b/src/Build/Evaluation/LazyItemEvaluator.cs index 58b2d1d661f..64df2a7402f 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.cs @@ -59,18 +59,14 @@ public LazyItemEvaluator(IEvaluatorData data, IItemFactory ite private ImmutableList GetItems(string itemType) { - LazyItemList itemList = GetItemList(itemType); - if (itemList == null) - { - return ImmutableList.Empty; - } - - return itemList.GetMatchedItems(ImmutableHashSet.Empty); + return _itemLists.TryGetValue(itemType, out LazyItemList itemList) ? + itemList.GetMatchedItems(ImmutableHashSet.Empty) : + ImmutableList.Empty; } public bool EvaluateConditionWithCurrentState(ProjectElement element, ExpanderOptions expanderOptions, ParserOptions parserOptions) { - return EvaluateCondition(element, expanderOptions, parserOptions, _expander, this); + return EvaluateCondition(element.Condition, element, expanderOptions, parserOptions, _expander, this); } private static bool EvaluateCondition( @@ -108,17 +104,6 @@ public bool EvaluateConditionWithCurrentState(ProjectElement element, ExpanderOp } } - private static bool EvaluateCondition( - ProjectElement element, - ExpanderOptions expanderOptions, - ParserOptions parserOptions, - Expander expander, - LazyItemEvaluator lazyEvaluator - ) - { - return EvaluateCondition(element.Condition, element, expanderOptions, parserOptions, expander, lazyEvaluator); - } - /// /// COMPAT: Whidbey used the "current project file/targets" directory for evaluating Import and PropertyGroup conditions /// Orcas broke this by using the current root project file for all conditions @@ -417,21 +402,13 @@ public OperationBuilderWithMetadata(ProjectItemElement itemElement, bool conditi private void AddReferencedItemList(string itemType, IDictionary referencedItemLists) { - var itemList = GetItemList(itemType); - if (itemList != null) + if (_itemLists.TryGetValue(itemType, out LazyItemList itemList)) { itemList.MarkAsReferenced(); referencedItemLists[itemType] = itemList; } } - private LazyItemList GetItemList(string itemType) - { - LazyItemList ret; - _itemLists.TryGetValue(itemType, out ret); - return ret; - } - public IEnumerable GetAllItemsDeferred() { return _itemLists.Values.SelectMany(itemList => itemList.GetItemData(ImmutableHashSet.Empty)) @@ -459,7 +436,7 @@ public void ProcessItemElement(string rootDirectory, ProjectItemElement itemElem ErrorUtilities.ThrowInternalErrorUnreachable(); } - LazyItemList previousItemList = GetItemList(itemElement.ItemType); + _itemLists.TryGetValue(itemElement.ItemType, out LazyItemList previousItemList); LazyItemList newList = new LazyItemList(previousItemList, operation); _itemLists[itemElement.ItemType] = newList; } diff --git a/src/Build/ObjectModelRemoting/DefinitionObjectsLinks/ProjectPropertyLink.cs b/src/Build/ObjectModelRemoting/DefinitionObjectsLinks/ProjectPropertyLink.cs index ee5c1263f3a..74fac4edbe2 100644 --- a/src/Build/ObjectModelRemoting/DefinitionObjectsLinks/ProjectPropertyLink.cs +++ b/src/Build/ObjectModelRemoting/DefinitionObjectsLinks/ProjectPropertyLink.cs @@ -68,7 +68,7 @@ public abstract class ProjectPropertyLink /// public static string GetEvaluatedValueEscaped(ProjectProperty property) { - return property.EvaluatedValueEscapedIntenral; + return property.EvaluatedValueEscapedInternal; } } } diff --git a/src/Build/ObjectModelRemoting/LinkedObjectFactory.cs b/src/Build/ObjectModelRemoting/LinkedObjectFactory.cs index 98ce259b750..2312e0d8be1 100644 --- a/src/Build/ObjectModelRemoting/LinkedObjectFactory.cs +++ b/src/Build/ObjectModelRemoting/LinkedObjectFactory.cs @@ -308,7 +308,7 @@ public override string UnevaluatedValue public override bool IsImported => Link.IsImported; - internal override string EvaluatedValueEscapedIntenral => Link.EvaluatedIncludeEscaped; + internal override string EvaluatedValueEscapedInternal => Link.EvaluatedIncludeEscaped; } #endregion } diff --git a/src/Build/Utilities/EngineFileUtilities.cs b/src/Build/Utilities/EngineFileUtilities.cs index b3c15748810..a58892f6c1b 100644 --- a/src/Build/Utilities/EngineFileUtilities.cs +++ b/src/Build/Utilities/EngineFileUtilities.cs @@ -167,7 +167,7 @@ private string[] GetFileList // Unescape before handing it to the filesystem. var directoryUnescaped = EscapingUtilities.UnescapeAll(directoryEscaped); var filespecUnescaped = EscapingUtilities.UnescapeAll(filespecEscaped); - var excludeSpecsUnescaped = excludeSpecsEscaped.Where(IsValidExclude).Select(EscapingUtilities.UnescapeAll).ToList(); + var excludeSpecsUnescaped = excludeSpecsEscaped.Where(IsValidExclude).Select(i => EscapingUtilities.UnescapeAll(i)).ToList(); // Get the list of actual files which match the filespec. Put // the list into a string array. If the filespec started out diff --git a/src/Shared/EscapingUtilities.cs b/src/Shared/EscapingUtilities.cs index ae7c5b67a1e..d934d8fab8d 100644 --- a/src/Shared/EscapingUtilities.cs +++ b/src/Shared/EscapingUtilities.cs @@ -36,6 +36,7 @@ private static bool IsHexDigit(char character) /// by the hexadecimal number XX. /// /// The string to unescape. + /// If the string should be trimmed before being unescaped. /// unescaped string internal static string UnescapeAll(string escapedString, bool trim = false) { From 4b42650a6476640c7698f30a6bc5db4f9750095f Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Wed, 26 Aug 2020 14:35:45 -0700 Subject: [PATCH 4/9] PR feedback --- src/Build/Evaluation/Evaluator.cs | 4 +--- src/Build/Evaluation/Expander.cs | 2 +- src/Shared/EscapingUtilities.cs | 6 +++--- src/Shared/FileUtilities.cs | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index a95105e72b3..3ea49c72406 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -1249,16 +1249,14 @@ private void AddToolsetProperties() /// private int AddGlobalProperties() { - int count = 0; if (_data.GlobalPropertiesDictionary != null) { foreach (ProjectPropertyInstance globalProperty in _data.GlobalPropertiesDictionary) { _data.SetProperty(globalProperty.Name, ((IProperty)globalProperty).EvaluatedValueEscaped, true /* IS global property */, false /* may NOT be a reserved name */); - count++; } } - return count; + return _data.GlobalPropertiesDictionary.Count; } /// diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index fa5a75cfa5a..41669240638 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1933,7 +1933,7 @@ private static class ItemExpander internal static string ExpandItemVectorsIntoString(Expander expander, string expression, IItemProvider items, ExpanderOptions options, IElementLocation elementLocation) where T : class, IItem { - if ((options & ExpanderOptions.ExpandItems) == 0) + if ((options & ExpanderOptions.ExpandItems) == 0 || expression.Length == 0) { return expression; } diff --git a/src/Shared/EscapingUtilities.cs b/src/Shared/EscapingUtilities.cs index d934d8fab8d..a8d86f06c31 100644 --- a/src/Shared/EscapingUtilities.cs +++ b/src/Shared/EscapingUtilities.cs @@ -52,7 +52,7 @@ internal static string UnescapeAll(string escapedString, bool trim = false) int indexOfPercent = escapedString.IndexOf('%'); if (indexOfPercent == -1) { - return escapedString; + return trim ? escapedString.Trim() : escapedString; } // This is where we're going to build up the final string to return to the caller. @@ -70,7 +70,7 @@ internal static string UnescapeAll(string escapedString, bool trim = false) { return String.Empty; } - while (Char.IsWhiteSpace(escapedString[escapedStringLength])) + while (Char.IsWhiteSpace(escapedString[escapedStringLength - 1])) { escapedStringLength--; } @@ -82,7 +82,7 @@ internal static string UnescapeAll(string escapedString, bool trim = false) // There must be two hex characters following the percent sign // for us to even consider doing anything with this. if ( - (indexOfPercent <= (escapedString.Length - 3)) && + (indexOfPercent <= (escapedStringLength - 3)) && IsHexDigit(escapedString[indexOfPercent + 1]) && IsHexDigit(escapedString[indexOfPercent + 2]) ) diff --git a/src/Shared/FileUtilities.cs b/src/Shared/FileUtilities.cs index fecfa69a3b7..38123159d53 100644 --- a/src/Shared/FileUtilities.cs +++ b/src/Shared/FileUtilities.cs @@ -234,7 +234,7 @@ internal static string EnsureNoLeadingOrTrailingSlash(string path) } int start = IsSlash(path[0]) ? 1 : 0; int end = IsSlash(path[path.Length - 1]) ? 1 : 0; - return path.Substring(start, path.Length - start - end); + return path.Length - start - end > 0 ? path.Substring(start, path.Length - start - end) : String.Empty; } /// From a008a27a6a93499c6f2412f05dafc1026bbbcc11 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Thu, 27 Aug 2020 19:35:05 -0700 Subject: [PATCH 5/9] PR feedback --- src/Build/Evaluation/Evaluator.cs | 6 +++--- src/Shared/FileUtilities.cs | 14 -------------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index 3ea49c72406..e46e67b0078 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -652,7 +652,7 @@ private void Evaluate() List initialTargets = new List(_initialTargetsList.Count); foreach (var initialTarget in _initialTargetsList) { - initialTargets.Add(EscapingUtilities.UnescapeAll(initialTarget, true)); + initialTargets.Add(EscapingUtilities.UnescapeAll(initialTarget, trim: true)); } _data.InitialTargets = initialTargets; @@ -964,7 +964,7 @@ private void UpdateDefaultTargets(ProjectRootElement currentProjectOrImport) for (int i = 0; i < temp.Count; i++) { - string target = EscapingUtilities.UnescapeAll(temp[i], true); + string target = EscapingUtilities.UnescapeAll(temp[i], trim: true); if (target.Length > 0) { _data.DefaultTargets ??= new List(temp.Count); @@ -1181,7 +1181,7 @@ private void AddBuiltInProperties() int rootLength = Path.GetPathRoot(projectDirectory).Length; string projectDirectoryNoRoot = projectDirectory.Substring(rootLength); - projectDirectoryNoRoot = FileUtilities.EnsureNoLeadingOrTrailingSlash(projectDirectoryNoRoot); + projectDirectoryNoRoot = projectDirectoryNoRoot.Trim(new char[] { '\\', '/' }); // ReservedPropertyNames.projectDefaultTargets is already set SetBuiltInProperty(ReservedPropertyNames.projectFile, projectFile); diff --git a/src/Shared/FileUtilities.cs b/src/Shared/FileUtilities.cs index 38123159d53..ca576a58036 100644 --- a/src/Shared/FileUtilities.cs +++ b/src/Shared/FileUtilities.cs @@ -223,20 +223,6 @@ internal static string EnsureNoTrailingSlash(string path) return path; } - /// - /// Ensures the path does not have a leading or trailing slash. - /// - internal static string EnsureNoLeadingOrTrailingSlash(string path) - { - if (String.IsNullOrEmpty(path)) - { - return path; - } - int start = IsSlash(path[0]) ? 1 : 0; - int end = IsSlash(path[path.Length - 1]) ? 1 : 0; - return path.Length - start - end > 0 ? path.Substring(start, path.Length - start - end) : String.Empty; - } - /// /// Indicates if the given file-spec ends with a slash. /// From 2d239b1a40c094e36e119889c16842d53ab5e08e Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Fri, 28 Aug 2020 08:27:10 -0700 Subject: [PATCH 6/9] No new array --- src/Build/Evaluation/Evaluator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index e46e67b0078..df604fc9a63 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -1181,7 +1181,7 @@ private void AddBuiltInProperties() int rootLength = Path.GetPathRoot(projectDirectory).Length; string projectDirectoryNoRoot = projectDirectory.Substring(rootLength); - projectDirectoryNoRoot = projectDirectoryNoRoot.Trim(new char[] { '\\', '/' }); + projectDirectoryNoRoot = projectDirectoryNoRoot.Trim(FileUtilities.Slashes); // ReservedPropertyNames.projectDefaultTargets is already set SetBuiltInProperty(ReservedPropertyNames.projectFile, projectFile); From f4766164d2239e589e2df38a131a543ebe0de49d Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Thu, 3 Sep 2020 09:05:45 -0700 Subject: [PATCH 7/9] FixFilePath --- src/Build/Evaluation/Evaluator.cs | 3 +-- src/Build/Evaluation/Expander.cs | 5 +---- src/Shared/FileUtilities.cs | 18 +++++++++++------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index df604fc9a63..daa5ed297f2 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -1180,8 +1180,7 @@ private void AddBuiltInProperties() string projectFullPath = Path.Combine(projectDirectory, projectFile); int rootLength = Path.GetPathRoot(projectDirectory).Length; - string projectDirectoryNoRoot = projectDirectory.Substring(rootLength); - projectDirectoryNoRoot = projectDirectoryNoRoot.Trim(FileUtilities.Slashes); + string projectDirectoryNoRoot = FileUtilities.EnsureNoLeadingOrTrailingSlash(projectDirectory, rootLength); // ReservedPropertyNames.projectDefaultTargets is already set SetBuiltInProperty(ReservedPropertyNames.projectFile, projectFile); diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 41669240638..c834a747db2 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1482,10 +1482,7 @@ private static object ExpandMSBuildThisFileProperty(string propertyName, IElemen { string directory = Path.GetDirectoryName(elementLocation.File); int rootLength = Path.GetPathRoot(directory).Length; - string directoryNoRoot = directory.Substring(rootLength); - directoryNoRoot = FileUtilities.EnsureTrailingSlash(directoryNoRoot); - directoryNoRoot = FileUtilities.EnsureNoLeadingSlash(directoryNoRoot); - value = directoryNoRoot; + value = FileUtilities.EnsureNoLeadingOrTrailingSlash(directory, rootLength); } return value; diff --git a/src/Shared/FileUtilities.cs b/src/Shared/FileUtilities.cs index ca576a58036..9f9900576f8 100644 --- a/src/Shared/FileUtilities.cs +++ b/src/Shared/FileUtilities.cs @@ -187,7 +187,7 @@ internal static void ClearCacheDirectory() internal static string EnsureTrailingSlash(string fileSpec) { fileSpec = FixFilePath(fileSpec); - if (fileSpec.Length > 0 && !EndsWithSlash(fileSpec)) + if (fileSpec.Length > 0 && !IsSlash(fileSpec[fileSpec.Length - 1])) { fileSpec += Path.DirectorySeparatorChar; } @@ -196,17 +196,21 @@ internal static string EnsureTrailingSlash(string fileSpec) } /// - /// Ensures the path does not have a leading slash. + /// Ensures the path does not have a leading or trailing slash after removing the first 'start' characters. /// - internal static string EnsureNoLeadingSlash(string path) + internal static string EnsureNoLeadingOrTrailingSlash(string path, int start) { - path = FixFilePath(path); - if (path.Length > 0 && IsSlash(path[0])) + int stop = path.Length; + while (start < stop && IsSlash(path[start])) + { + start++; + } + while (start < stop && IsSlash(path[stop - 1])) { - path = path.Substring(1); + stop--; } - return path; + return FixFilePath(path.Substring(start, stop - start)); } /// From 05272f85a5112311da7155f2aec66c1d718eb966 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Thu, 3 Sep 2020 13:50:47 -0700 Subject: [PATCH 8/9] Add FileUtilities method to ensure trailing but not leading slash I misread Expander.cs's use of this, so they can't use the same method, but this retains the efficiency while only adding a little code. --- src/Build/Evaluation/Expander.cs | 2 +- src/Shared/FileUtilities.cs | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index c834a747db2..bad39479a75 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1482,7 +1482,7 @@ private static object ExpandMSBuildThisFileProperty(string propertyName, IElemen { string directory = Path.GetDirectoryName(elementLocation.File); int rootLength = Path.GetPathRoot(directory).Length; - value = FileUtilities.EnsureNoLeadingOrTrailingSlash(directory, rootLength); + value = FileUtilities.EnsureTrailingNoLeadingSlash(directory, rootLength); } return value; diff --git a/src/Shared/FileUtilities.cs b/src/Shared/FileUtilities.cs index 9f9900576f8..6e6f789c2c1 100644 --- a/src/Shared/FileUtilities.cs +++ b/src/Shared/FileUtilities.cs @@ -213,6 +213,22 @@ internal static string EnsureNoLeadingOrTrailingSlash(string path, int start) return FixFilePath(path.Substring(start, stop - start)); } + /// + /// Ensures the path does not have a leading slash after removing the first 'start' characters but does end in a slash. + /// + internal static string EnsureTrailingNoLeadingSlash(string path, int start) + { + int stop = path.Length; + while (start < stop && IsSlash(path[start])) + { + start++; + } + + return FixFilePath(start < stop && IsSlash(path[stop - 1]) ? + path.Substring(start) : + path.Substring(start) + Path.DirectorySeparatorChar); + } + /// /// Ensures the path does not have a trailing slash. /// From 57e230f44b8c6af642f0214295f1e74b2378286e Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Fri, 4 Sep 2020 09:43:29 -0700 Subject: [PATCH 9/9] Prevent NRE --- src/Build/Evaluation/Evaluator.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index daa5ed297f2..8e10b13577b 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -1248,13 +1248,16 @@ private void AddToolsetProperties() /// private int AddGlobalProperties() { - if (_data.GlobalPropertiesDictionary != null) + if (_data.GlobalPropertiesDictionary == null) { - foreach (ProjectPropertyInstance globalProperty in _data.GlobalPropertiesDictionary) - { - _data.SetProperty(globalProperty.Name, ((IProperty)globalProperty).EvaluatedValueEscaped, true /* IS global property */, false /* may NOT be a reserved name */); - } + return 0; + } + + foreach (ProjectPropertyInstance globalProperty in _data.GlobalPropertiesDictionary) + { + _data.SetProperty(globalProperty.Name, ((IProperty)globalProperty).EvaluatedValueEscaped, true /* IS global property */, false /* may NOT be a reserved name */); } + return _data.GlobalPropertiesDictionary.Count; }