Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Evaluate #5676

Merged
merged 9 commits into from Sep 11, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Build/Definition/ProjectProperty.cs
Expand Up @@ -49,7 +49,7 @@ internal ProjectProperty(Project project, string evaluatedValueEscaped)
_evaluatedValueEscaped = evaluatedValueEscaped;
}

internal virtual string EvaluatedValueEscapedIntenral => _evaluatedValueEscaped;
internal virtual string EvaluatedValueEscapedInternal => _evaluatedValueEscaped;

/// <summary>
/// Name of the property.
Expand Down Expand Up @@ -79,7 +79,7 @@ public string EvaluatedValue
{
[DebuggerStepThrough]
get
{ return EscapingUtilities.UnescapeAll(EvaluatedValueEscapedIntenral); }
{ return EscapingUtilities.UnescapeAll(EvaluatedValueEscapedInternal); }
}

/// <summary>
Expand All @@ -94,7 +94,7 @@ public string EvaluatedValue
string IProperty.EvaluatedValueEscaped
{
[DebuggerStepThrough]
get => EvaluatedValueEscapedIntenral;
get => EvaluatedValueEscapedInternal;
}

/// <summary>
Expand Down Expand Up @@ -201,7 +201,7 @@ string IKeyed.Key
string IValued.EscapedValue
{
[DebuggerStepThrough]
get => EvaluatedValueEscapedIntenral;
get => EvaluatedValueEscapedInternal;
}

#region IEquatable<ProjectProperty> Members
Expand All @@ -225,7 +225,7 @@ bool IEquatable<ProjectProperty>.Equals(ProjectProperty other)

return _project == other._project &&
Xml == other.Xml &&
EvaluatedValueEscapedIntenral == other.EvaluatedValueEscapedIntenral &&
EvaluatedValueEscapedInternal == other.EvaluatedValueEscapedInternal &&
Name == other.Name;
}

Expand Down
130 changes: 51 additions & 79 deletions src/Build/Evaluation/Evaluator.cs

Large diffs are not rendered by default.

7 changes: 1 addition & 6 deletions src/Build/Evaluation/Expander.cs
Expand Up @@ -738,11 +738,6 @@ internal static string ExpandMetadataLeaveEscaped(string expression, IMetadataTa
return expression;
}

if (expression.Length == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why can this be removed?

{
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
Expand Down Expand Up @@ -1938,7 +1933,7 @@ private static class ItemExpander
internal static string ExpandItemVectorsIntoString<T>(Expander<P, I> expander, string expression, IItemProvider<T> items, ExpanderOptions options, IElementLocation elementLocation)
where T : class, IItem
{
if (((options & ExpanderOptions.ExpandItems) == 0) || (expression.Length == 0))
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this condition was not helping? FWIW, the caller is not performing this check.

Copy link
Member Author

Choose a reason for hiding this comment

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

The caller checks the length before it expands metadata and properties. If it's just a property or just metadata, and it happens to be empty, there could be an extra string before this, but I think that's the only case. Still valid, so I added it back.

if ((options & ExpanderOptions.ExpandItems) == 0)
{
return expression;
}
Expand Down
4 changes: 1 addition & 3 deletions src/Build/Evaluation/ItemSpec.cs
Expand Up @@ -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
{
Expand Down Expand Up @@ -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()
Expand Down
35 changes: 6 additions & 29 deletions src/Build/Evaluation/LazyItemEvaluator.cs
Expand Up @@ -59,18 +59,14 @@ public LazyItemEvaluator(IEvaluatorData<P, I, M, D> data, IItemFactory<I, I> ite

private ImmutableList<I> GetItems(string itemType)
{
LazyItemList itemList = GetItemList(itemType);
if (itemList == null)
{
return ImmutableList<I>.Empty;
}

return itemList.GetMatchedItems(ImmutableHashSet<string>.Empty);
return _itemLists.TryGetValue(itemType, out LazyItemList itemList) ?
itemList.GetMatchedItems(ImmutableHashSet<string>.Empty) :
ImmutableList<I>.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(
Expand Down Expand Up @@ -108,17 +104,6 @@ public bool EvaluateConditionWithCurrentState(ProjectElement element, ExpanderOp
}
}

private static bool EvaluateCondition(
ProjectElement element,
ExpanderOptions expanderOptions,
ParserOptions parserOptions,
Expander<P, I> expander,
LazyItemEvaluator<P, I, M, D> lazyEvaluator
)
{
return EvaluateCondition(element.Condition, element, expanderOptions, parserOptions, expander, lazyEvaluator);
}

/// <summary>
/// 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
Expand Down Expand Up @@ -417,21 +402,13 @@ public OperationBuilderWithMetadata(ProjectItemElement itemElement, bool conditi

private void AddReferencedItemList(string itemType, IDictionary<string, LazyItemList> 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<ItemData> GetAllItemsDeferred()
{
return _itemLists.Values.SelectMany(itemList => itemList.GetItemData(ImmutableHashSet<string>.Empty))
Expand Down Expand Up @@ -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;
}
Expand Down
3 changes: 0 additions & 3 deletions src/Build/Microsoft.Build.csproj
Expand Up @@ -696,9 +696,6 @@
<Compile Include="..\Shared\ExceptionHandling.cs">
<Link>SharedUtilities\ExceptionHandling.cs</Link>
</Compile>
<Compile Include="..\Shared\EscapingStringExtensions\EscapingStringExtensions.cs">
<Link>SharedUtilities\EscapingStringExtensions\EscapingStringExtensions.cs</Link>
</Compile>
<Compile Include="..\Shared\FileMatcher.cs">
<Link>SharedUtilities\FileMatcher.cs</Link>
<ExcludeFromStyleCop>true</ExcludeFromStyleCop>
Expand Down
Expand Up @@ -68,7 +68,7 @@ public abstract class ProjectPropertyLink
/// </summary>
public static string GetEvaluatedValueEscaped(ProjectProperty property)
{
return property.EvaluatedValueEscapedIntenral;
return property.EvaluatedValueEscapedInternal;
}
}
}
2 changes: 1 addition & 1 deletion src/Build/ObjectModelRemoting/LinkedObjectFactory.cs
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

If this was in its own commit it'd be a lot easier to glance over, see that it's fine, and then ignore.

}
#endregion
}
Expand Down
2 changes: 1 addition & 1 deletion src/Build/Utilities/EngineFileUtilities.cs
Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions src/Build/Utilities/FileSpecMatchTester.cs
Expand Up @@ -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;
Expand All @@ -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);
Expand Down
34 changes: 0 additions & 34 deletions src/Shared/EscapingStringExtensions/EscapingStringExtensions.cs

This file was deleted.

44 changes: 19 additions & 25 deletions src/Shared/EscapingUtilities.cs
Expand Up @@ -24,20 +24,6 @@ static internal class EscapingUtilities
/// </summary>
private static Dictionary<string, string> s_unescapedToEscapedStrings = new Dictionary<string, string>(StringComparer.Ordinal);

/// <summary>
/// Replaces all instances of %XX in the input string with the character represented
/// by the hexadecimal number XX.
/// </summary>
/// <param name="escapedString">The string to unescape.</param>
/// <returns>unescaped string</returns>
internal static string UnescapeAll
(
string escapedString
)
{
return UnescapeAll(escapedString, out bool _);
}

private static bool IsHexDigit(char character)
{
return ((character >= '0') && (character <= '9'))
Expand All @@ -50,16 +36,10 @@ private static bool IsHexDigit(char character)
/// by the hexadecimal number XX.
/// </summary>
/// <param name="escapedString">The string to unescape.</param>
/// <param name="escapingWasNecessary">Whether any replacements were made.</param>
/// <param name="trim">If the string should be trimmed before being unescaped.</param>
/// <returns>unescaped string</returns>
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))
Expand All @@ -79,6 +59,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]))
cdmihai marked this conversation as resolved.
Show resolved Hide resolved
{
currentPosition++;
}
if (currentPosition == escapedString.Length)
{
return String.Empty;
}
while (Char.IsWhiteSpace(escapedString[escapedStringLength]))
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look correct. The last character is at index escapedStringLength - 1.

Copy link
Member

Choose a reason for hiding this comment

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

Also, line 85:

(indexOfPercent <= (escapedStringLength - 3)) &&

instead of:

(indexOfPercent <= (escapedString.Length - 3)) &&

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, thanks! No reason to check that digits are hex if they're white space.

{
escapedStringLength--;
}
Copy link
Member

Choose a reason for hiding this comment

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

side-note: #5663 includes a Trim() implementation operating on spans which could potentially be used here.

Copy link
Contributor

@Therzok Therzok Aug 27, 2020

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, the .NET Core implementation of ReadOnlySpan<char> does not recover the original string object in ToString() even if it spans the entire string. This is handled in #5663 by keeping track of the string reference separately so it can be returned if the span hasn't changed.

}

// Loop until there are no more percent signs in the input string.
while (indexOfPercent != -1)
Expand Down Expand Up @@ -106,8 +102,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.
Expand All @@ -116,7 +110,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);
}
Expand Down
14 changes: 14 additions & 0 deletions src/Shared/FileUtilities.cs
Expand Up @@ -223,6 +223,20 @@ internal static string EnsureNoTrailingSlash(string path)
return path;
}

/// <summary>
/// Ensures the path does not have a leading or trailing slash.
/// </summary>
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);
Copy link
Member

Choose a reason for hiding this comment

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

This would blow up on path.Substring(1, -1) if the function was passed "/" as the path argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

}

/// <summary>
/// Indicates if the given file-spec ends with a slash.
/// </summary>
Expand Down
3 changes: 0 additions & 3 deletions src/Utilities/Microsoft.Build.Utilities.csproj
Expand Up @@ -88,9 +88,6 @@
<Compile Include="..\Shared\ExceptionHandling.cs">
<Link>Shared\ExceptionHandling.cs</Link>
</Compile>
<Compile Include="..\Shared\EscapingStringExtensions\EscapingStringExtensions.cs">
<Link>Shared\EscapingStringExtensions\EscapingStringExtensions.cs</Link>
</Compile>
<Compile Include="..\Shared\FileUtilities.cs">
<Link>Shared\FileUtilities.cs</Link>
</Compile>
Expand Down