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 4 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
128 changes: 49 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 || expression.Length == 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.

48 changes: 21 additions & 27 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 @@ -72,21 +52,37 @@ internal static string UnescapeAll
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.
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 - 1]))
{
escapedStringLength--;
}
}

// Loop until there are no more percent signs in the input string.
while (indexOfPercent != -1)
{
// 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])
)
Expand All @@ -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.Length - start - end > 0 ? path.Substring(start, path.Length - start - end) : String.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if there's a more elegant way to express this, the best I could come up with is:

Suggested change
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;
int start = IsSlash(path[0]) ? 1 : 0;
if (path.Length == 1)
{
return path.Substring(start);
}
int end = IsSlash(path[path.Length - 1]) ? 1 : 0;
return path.Substring(start, path.Length - start - end);

Probably not any better, though.

Copy link
Contributor

@cdmihai cdmihai Aug 28, 2020

Choose a reason for hiding this comment

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

Why not reuse String.Trim(Char[]) to implement this?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually really nice. Thanks for the suggestion!

Copy link
Member

@ladipro ladipro Aug 28, 2020

Choose a reason for hiding this comment

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

String.Trim() removes all leading/trailing characters from the set, though. This behavior matches the name of the method but it's not what the method was doing. Is it guaranteed to not break anything?

Example: "\\hello"
Previously it would become "\hello", now after calling String.Trim() it is "hello".

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm reasonably convinced that if there were multiple slashes on either side, that was a mistake because this is used for a path; there's no reason (unless you're explicitly trying to hack around a flawed implementation of EnsureNoLeadingSlash) to have multiple slashes in a row.

Additionally, to comment on the fact that I no longer escaped projectDirectoryNoRoot: it was already escaped just above there, so I believe that was extra work to no benefit. If there is a reason that double-escaping something (and presumably double-unescaping something) is preferred, I can revert that part.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thank you. How about the slash normalization done in FixFilePath() and called from EnsureNoLeadingSlash() and EnsureNoTrailingSlash()? Is it also something we know for sure wasn't needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not confident about that and would actually guess that might be problematic, although the fact that it passed tests suggests it might be ok. To be safe, I'll add it back. Good catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

And while I'm at it, I'm not sure why I should have to substring, then trim. Why not both at once?

}

/// <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