Optimize Evaluate #5676

Forgind · 2020-08-25T01:18:27Z

In retrospect, this should have included the second commit from #5670, making that one just one commit. I can do that if you'd like, though I'd slightly prefer to keep it as-is just because that's less effort for me. :)

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.

Also inlined GetItemList and one of the private overloads of EvaluateCondition

ladipro

Looks great! I've added a few comments inline.

ladipro · 2020-08-26T11:07:01Z

src/Shared/FileUtilities.cs

+            }
+            int start = IsSlash(path[0]) ? 1 : 0;
+            int end = IsSlash(path[path.Length - 1]) ? 1 : 0;
+            return path.Substring(start, path.Length - start - end);


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

Fixed, thanks!

ladipro · 2020-08-26T11:10:08Z

src/Build/Evaluation/Evaluator.cs

        }

        /// <summary>
        /// Put all the global properties into our property bag
        /// </summary>
-        private ICollection<P> AddGlobalProperties()
+        private int AddGlobalProperties()


❤️ for all changes in this file. You are awesome.

ladipro · 2020-08-26T11:20:44Z

src/Build/Evaluation/Evaluator.cs

            }
-
-            return globalProperties;
+            return count;


nit: Just return _data.GlobalPropertiesDictionary.Count?

I hesitated briefly because it technically would change the behavior if there are already properties in that dictionary, which we could add, but then your way works and the current way doesn't, so it's a change we would make anyway. Thanks for the suggestion!

ladipro · 2020-08-26T11:25:42Z

src/Build/Evaluation/Expander.cs

@@ -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))


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

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.

ladipro · 2020-08-26T11:58:28Z

src/Shared/EscapingUtilities.cs

+                {
+                    return String.Empty;
+                }
+                while (Char.IsWhiteSpace(escapedString[escapedStringLength]))


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

Also, line 85:

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

instead of:

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

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

ladipro · 2020-08-26T12:03:55Z

src/Shared/EscapingUtilities.cs

+                while (currentPosition < escapedString.Length && Char.IsWhiteSpace(escapedString[currentPosition]))
+                {
+                    currentPosition++;
+                }
+                if (currentPosition == escapedString.Length)
+                {
+                    return String.Empty;
+                }
+                while (Char.IsWhiteSpace(escapedString[escapedStringLength]))
+                {
+                    escapedStringLength--;
+                }


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

One thing to note is that if Trim() does not modify the span, and ToString() is done on the span, it will allocate a new string:

https://github.com/Therzok/BenchPlayground/blob/820035919d2a47506841041fe7caea799dc939ea/BenchPlayground/Benchmarks/runtime/SpanToString.md
https://github.com/Therzok/BenchPlayground/blob/820035919d2a47506841041fe7caea799dc939ea/BenchPlayground/Benchmarks/runtime/SpanToString.cs

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.

ladipro · 2020-08-27T10:15:01Z

src/Shared/FileUtilities.cs

+            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;


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.

src/Shared/EscapingUtilities.cs

cdmihai · 2020-08-28T00:40:20Z

src/Build/Evaluation/Evaluator.cs

@@ -652,7 +652,7 @@ private void Evaluate()
                List<string> initialTargets = new List<string>(_initialTargetsList.Count);
                foreach (var initialTarget in _initialTargetsList)
                {
-                    initialTargets.Add(EscapingUtilities.UnescapeAll(initialTarget.Trim()));
+                    initialTargets.Add(EscapingUtilities.UnescapeAll(initialTarget, true));


nit: I prefer to include the parameter name for bool literal arguments to make it clear what the bool means without having to look at the signature of the called method.

cdmihai · 2020-08-28T00:51:49Z

src/Shared/FileUtilities.cs

+            }
+            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;


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

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

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".

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.

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?

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!

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

cdmihai

LGTM pending comments.

ladipro · 2020-08-28T07:05:02Z

src/Build/Evaluation/Evaluator.cs


                int rootLength = Path.GetPathRoot(projectDirectory).Length;
                string projectDirectoryNoRoot = projectDirectory.Substring(rootLength);
-                projectDirectoryNoRoot = FileUtilities.EnsureNoTrailingSlash(projectDirectoryNoRoot);
-                projectDirectoryNoRoot = EscapingUtilities.Escape(FileUtilities.EnsureNoLeadingSlash(projectDirectoryNoRoot));
+                projectDirectoryNoRoot = projectDirectoryNoRoot.Trim(new char[] { '\\', '/' });


nit: FileUtilities.Slashes instead of allocating a new array.

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.

rainersigwald · 2020-08-27T22:10:47Z

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;


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

src/Build/Evaluation/Evaluator.cs

rainersigwald · 2020-08-27T22:32:20Z

src/Build/Evaluation/Expander.cs

@@ -738,11 +738,6 @@ internal static string ExpandMetadataLeaveEscaped(string expression, IMetadataTa
                        return expression;
                    }

-                    if (expression.Length == 0)


Why can this be removed?

Forgind added 3 commits August 24, 2020 15:10

Passes 0 and 1 Optimizations

8b59434

Optimize pass 2

07a77e0

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.

cleanup compile errors

1e1939c

Also inlined GetItemList and one of the private overloads of EvaluateCondition

ladipro reviewed Aug 26, 2020

View reviewed changes

PR feedback

4b42650

ladipro approved these changes Aug 27, 2020

View reviewed changes

cdmihai reviewed Aug 28, 2020

View reviewed changes

src/Shared/EscapingUtilities.cs Show resolved Hide resolved

cdmihai reviewed Aug 28, 2020

View reviewed changes

cdmihai approved these changes Aug 28, 2020

View reviewed changes

PR feedback

a008a27

ladipro reviewed Aug 28, 2020

View reviewed changes

Forgind added 3 commits August 28, 2020 08:27

No new array

2d239b1

FixFilePath

f476616

Add FileUtilities method to ensure trailing but not leading slash

05272f8

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.

rainersigwald reviewed Sep 4, 2020

View reviewed changes

Prevent NRE

57e230f

rainersigwald approved these changes Sep 11, 2020

View reviewed changes

Forgind merged commit f577a68 into dotnet:master Sep 11, 2020

Forgind deleted the refactor branch September 11, 2020 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize Evaluate #5676

Optimize Evaluate #5676

Forgind commented Aug 25, 2020

ladipro left a comment

ladipro Aug 26, 2020

Forgind Aug 26, 2020

ladipro Aug 26, 2020

ladipro Aug 26, 2020

Forgind Aug 26, 2020

ladipro Aug 26, 2020

Forgind Aug 26, 2020

ladipro Aug 26, 2020

ladipro Aug 26, 2020

Forgind Aug 26, 2020

ladipro Aug 26, 2020

Therzok Aug 27, 2020 •

edited

ladipro Aug 31, 2020

ladipro Aug 27, 2020

cdmihai Aug 28, 2020

cdmihai Aug 28, 2020 •

edited

Forgind Aug 28, 2020

ladipro Aug 28, 2020 •

edited

Forgind Aug 28, 2020

ladipro Aug 31, 2020

Forgind Sep 3, 2020

Forgind Sep 3, 2020

cdmihai left a comment

ladipro Aug 28, 2020

rainersigwald Aug 27, 2020

rainersigwald Aug 27, 2020

Optimize Evaluate #5676

Optimize Evaluate #5676

Conversation

Forgind commented Aug 25, 2020

ladipro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Therzok Aug 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Aug 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladipro Aug 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Therzok Aug 27, 2020 •

edited

cdmihai Aug 28, 2020 •

edited

ladipro Aug 28, 2020 •

edited