Eval cond perf #6859

Forgind · 2021-09-15T23:35:01Z

This incorporates several changes to try to speed up the average time it takes to evaluate conditions. Specifically, it attempts to avoid repeat computations by combining Can...Evaluate and ...Evaluate into Try...Evaluate, only checks whether something is empty once each, and exits early if it's clearly empty or not empty. Look at this commit-by-commit! Also note that commits 2+3 should be combined, commits 4 and 7 cancel each other out, and commits 5+8 should be combined. I also will need to add comments for the new Try*Evaluate methods, but I think this is otherwise ready.

Edit: squashed this down to 3 cleaner commits.

Testing

I tried building OrchardCore before and after this change. EvaluateCondition seemed to take about 10% of total Evaluate time before, and this shaved off about a third of that.

Notes

Forgind · 2021-09-17T17:22:42Z

I'm looking at the last test failure, and it seems to be for a condition like:
('a'=='a')==a

The first part of that evaluates fine, but the second part doesn't evaluate and is supposed to throw an "improperly constructed" exception.

This change skips over evaluating the second properly because it recognizes it as a non-bool and says they aren't equal. I can change it to maintain exact behavior of what we had before, but this doesn't seem particularly worse to me. Thoughts?

Forgind · 2021-09-17T17:25:53Z

src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs

+                double? rightNumericValue = RightChild.TryNumericEvaluate(state);
+                if (rightNumericValue is not null)
+                {
+                    return Compare(leftNumericValue.Value, rightNumericValue);
+                }


I don't know if there are other cases similar to MSBuildToolsVersion. I would guess not, at a glance. Maybe make this:

Suggested change

double? rightNumericValue = RightChild.TryNumericEvaluate(state);

if (rightNumericValue is not null)

{

return Compare(leftNumericValue.Value, rightNumericValue);

}

double? rightNumericValue = RightChild.TryNumericEvaluate(state);

if (rightNumericValue is not null)

{

return Compare(leftNumericValue.Value, rightNumericValue);

}

else if (!LeftChild.DebuggerDisplay.Equals("$(MSBuildToolsVersion)")

{

return false;

}

benvillalobos · 2021-09-17T17:59:00Z

This change skips over evaluating the second properly because it recognizes it as a non-bool and says they aren't equal.

It'll be harder to get this PR in if it's meant for perf but we've changed something observable to customers (outside of faster builds).

What if we consider this PR a perf improvement and modify it a bit for #6277? It sounds like this PR could set the stage for that other issue.

ladipro · 2021-09-20T14:06:27Z

src/Shared/ConversionUtilities.cs

+            double? returnValue = TryConvertDecimalOrHexToDouble(number);
+            if (returnValue is null)


nit: I wonder if you can use the conventional Try* pattern here for a more readable (my opinion) code.

Suggested change

double? returnValue = TryConvertDecimalOrHexToDouble(number);

if (returnValue is null)

if (!TryConvertDecimalOrHexToDouble(number, out double returnValue))

Same for all the TryConvert* methods you're introducing.

ladipro · 2021-09-20T14:17:35Z

src/Build/Evaluation/Conditionals/StringExpressionNode.cs

+                    if (_value.Length > 2)
+                    {
+                        if (_value[1] != '(' || _value[_value.Length - 1] != ')' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@'))
+                        {
+                            // This isn't just a property, item, or metadata value, and it isn't empty.
+                            return false;
+                        }
+                    }
+                    else if (_value.Length == 0)
+                    {
+                        _cachedExpandedValue = String.Empty;
+                        return true;
+                    }
+                    else
+                    {
+                        _cachedExpandedValue = _value;
+                        return false;
+                    }


nit: Maybe use a switch? And/or comment, the last else block is especially hard to understand.

Suggested change

if (_value.Length > 2)

{

if (_value[1] != '(' || _value[_value.Length - 1] != ')' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@'))

{

// This isn't just a property, item, or metadata value, and it isn't empty.

return false;

}

}

else if (_value.Length == 0)

{

_cachedExpandedValue = String.Empty;

return true;

}

else

{

_cachedExpandedValue = _value;

return false;

}

switch (_value.Length)

{

case 0: _cachedExpandedValue = String.Empty; return true;

case 1:

case 2: _cachedExpandedValue = _value; return false;

default:

{

if (_value[1] != '(' || _value[_value.Length - 1] != ')' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@'))

{

// This isn't just a property, item, or metadata value, and it isn't empty.

return false;

}

}

}

Sounds good! The 1/2 case is for if it's nonempty but not long enough to have @/$/%, (, and ), so it has some characters that stay as they are. I can add a comment to make that clearer.

ladipro · 2021-09-20T14:30:42Z

src/Build/Evaluation/Conditionals/NotEqualExpressionNode.cs

        {
-            return left != right;
+            return right is null || left != right.Value;


nit: I'd check this in the caller to avoid the asymmetry. Should be easy if you switch to the bool Try(.., out value) pattern suggested in my other comment.

When calculating a value to see if we can do a type of comparison, store that value so we don't have to calculate it again. Use Try*Evaluate instead of Can*Evaluate and *Evaluate.

Forgind · 2021-10-04T19:36:37Z

I tried putting in the "throw error" commit (which I think should resolve the error problem), and although I only ran a few tests, it was faster than without it. It doesn't make sense to me, since it should be defaulting to a string comparison more often, but ¯\(ツ)/¯ It's hard to argue with both more correct and faster.

KirillOsenkov · 2021-10-11T01:39:06Z

src/Build/Evaluation/ExpressionShredder.cs

@@ -110,12 +110,14 @@ internal static List<ItemExpressionCapture> GetReferencedItemExpressions(string
        {
            List<ItemExpressionCapture> subExpressions = null;

-            if (expression.IndexOf('@') < 0)
+            int startInd = expression.IndexOf('@', start, end - start);


ideally we should avoid abbreviations, so startIndex would be better in my opinion

rainersigwald · 2021-10-11T14:12:00Z

src/Build/Evaluation/Conditionals/StringExpressionNode.cs

+                            _cachedExpandedValue = _value;
+                            return false;
+                        default:
+                            if (_value[1] != '(' || _value[_value.Length - 1] != ')' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@'))


I suspect it's totally irrelevant here, but this set off my nano-optimization sense. I suspect that if this were a critically hot loop, and depending on input characteristics, you might observe a speedup with this reordering:

Suggested change

if (_value[1] != '(' || _value[_value.Length - 1] != ')' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@'))

if (_value[1] != '(' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@') || _value[_value.Length - 1] != ')')

For a long _value the CPU might have to fault in the memory for the end of the string when accessing it, but we're guaranteed that the second character of the string was loaded at the same time as the first, so this can avoid cache misses.

Our strings are usually short so this probably won't generally matter, and even if it did it probably wouldn't matter much. But I know this kind of thing is up your alley so I figured I'd mention it :)

rainersigwald · 2021-10-11T14:14:08Z

src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs

+                // The left child evaluating to a number and the right child not evaluating to a number
+                // is insufficient to say they are not equal because $(MSBuildToolsVersion) evaluates to
+                // the string "Current" most of the time but when doing numeric comparisons, is treated
+                // as a version and returns "17.0" (or whatever the current tools version is). This means
+                // that if '$(MSBuildToolsVersion)' is "equal" to BOTH '17.0' and 'Current' (if 'Current'
+                // is 17.0).


Nice comment 👍🏻

rainersigwald · 2021-10-11T14:20:29Z

src/Build/Evaluation/Conditionals/StringExpressionNode.cs

+
+        internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result)
+        {
+            if (ShouldBeTreatedAsVisualStudioVersion(state))


Should NumericEvaluate be reimplemented in terms of TryNumericEvaluate to reduce code duplication?

Or alternately should we entirely remove the non-Try versions?

Removes all *Evaluate and Can*Evaluate except in OperatorExpressionNodes, for which CanBoolEvaluate always returns true, so using a simple BoolEvaluate makes sense. Note that for the NumericComparisonExpressionNode, we could save a tiny amount of time by only calling TryEvaluate when it's actually used (and using if/else as before), but that would since something can be both a number and a version, that would mean we would have to check both regardless of what we had previously found, which means it would be noticeably messier for (probably) very little perf gain, so I opted against that. Switch statements are so pretty 🙂

Apparently that's the best supported way

I tested once with and once without this change, and with this change, Evaluate was 3% faster...that sounds noisy, but I'll still take it.

ladipro

The change looks great. I've left a few comments related to what you brought up in our 1:1 chat. There are places where we evaluate arguments which are used only in error cases. These calls don't look super expensive but they're avoidable nonetheless (ex: NotExpressionNode.GetExpandedValue() allocates a string). Thank you!

ladipro · 2021-10-22T12:34:06Z

src/Build/Evaluation/Conditionals/NotExpressionNode.cs

+            ProjectErrorUtilities.VerifyThrowInvalidProject
+                    (LeftChild.TryBoolEvaluate(state, out bool boolValue),
+                     state.ElementLocation,
+                     "ExpressionDoesNotEvaluateToBoolean",
+                     LeftChild.GetUnexpandedValue(state),
+                     LeftChild.GetExpandedValue(state),
+                     state.Condition);


Suggested change

ProjectErrorUtilities.VerifyThrowInvalidProject

(LeftChild.TryBoolEvaluate(state, out bool boolValue),

state.ElementLocation,

"ExpressionDoesNotEvaluateToBoolean",

LeftChild.GetUnexpandedValue(state),

LeftChild.GetExpandedValue(state),

state.Condition);

if (!LeftChild.TryBoolEvaluate(state, out bool boolValue))

{

ProjectErrorUtilities.ThrowInvalidProject

state.ElementLocation,

"ExpressionDoesNotEvaluateToBoolean",

LeftChild.GetUnexpandedValue(state),

LeftChild.GetExpandedValue(state),

state.Condition);

}

to avoid the GetUnexpandedValue() and GetExpandedValue() calls in the happy case, as discussed.

Just finished pushing the VerifyThrow --> Throw change in the other PR.

ladipro · 2021-10-22T12:39:02Z

src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs

+                 isLeftNum ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state),
+                 isLeftNum ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state));


Also probably worth optimizing by not making these calls in the happy case.

ladipro · 2021-10-22T12:43:49Z

src/Build/Evaluation/Conditionals/OrExpressionNode.cs

                     LeftChild.GetUnexpandedValue(state),
                     LeftChild.GetExpandedValue(state),


These calls can be avoided in the happy case.

ladipro · 2021-10-22T12:44:29Z

src/Build/Evaluation/Conditionals/OrExpressionNode.cs

                     RightChild.GetUnexpandedValue(state),
                     RightChild.GetExpandedValue(state),


These calls can be avoided in the happy case.

Forgind commented Sep 17, 2021

View reviewed changes

ladipro reviewed Sep 20, 2021

View reviewed changes

Forgind added 3 commits September 20, 2021 09:45

Early exit

e02b3d9

Prevent double computation

64354ff

When calculating a value to see if we can do a type of comparison, store that value so we don't have to calculate it again. Use Try*Evaluate instead of Can*Evaluate and *Evaluate.

Use search for @

762ac1d

Forgind force-pushed the eval-cond-perf branch from b0290cc to 762ac1d Compare September 20, 2021 16:52

Forgind added 2 commits September 22, 2021 16:41

Fix incorrect bool check

f9456c4

Throw error

16a30b3

KirillOsenkov reviewed Oct 11, 2021

View reviewed changes

rainersigwald approved these changes Oct 11, 2021

View reviewed changes

Forgind added 5 commits October 11, 2021 10:17

Do not abbreviate variable name

2aac6e2

Switch switch to tuple

0134385

Apparently that's the best supported way

Switch order of check

625fd3f

I tested once with and once without this change, and with this change, Evaluate was 3% faster...that sounds noisy, but I'll still take it.

Throw exception where necessary

62760c1

ladipro approved these changes Oct 22, 2021

View reviewed changes

Forgind mentioned this pull request Oct 25, 2021

Simplify verify throw and remove unnecessary usings #6992

Closed

rainersigwald added this to the MSBuild 17.1 milestone Oct 25, 2021

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Oct 25, 2021

rokonec merged commit a80e227 into dotnet:main Oct 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eval cond perf #6859

Eval cond perf #6859

Forgind commented Sep 15, 2021 •

edited

Loading

Forgind commented Sep 17, 2021

Forgind Sep 17, 2021 •

edited

Loading

benvillalobos commented Sep 17, 2021

ladipro Sep 20, 2021

ladipro Sep 20, 2021

Forgind Sep 20, 2021

ladipro Sep 20, 2021

Forgind commented Oct 4, 2021 •

edited

Loading

KirillOsenkov Oct 11, 2021

rainersigwald Oct 11, 2021

rainersigwald Oct 11, 2021

rainersigwald Oct 11, 2021

rainersigwald Oct 11, 2021

ladipro left a comment

ladipro Oct 22, 2021

Forgind Oct 25, 2021

ladipro Oct 22, 2021

ladipro Oct 22, 2021

ladipro Oct 22, 2021

		double? returnValue = TryConvertDecimalOrHexToDouble(number);
		if (returnValue is null)

	double? returnValue = TryConvertDecimalOrHexToDouble(number);
	if (returnValue is null)
	if (!TryConvertDecimalOrHexToDouble(number, out double returnValue))

	if (_value[1] != '(' \|\| _value[_value.Length - 1] != ')' \|\| (_value[0] != '$' && _value[0] != '%' && _value[0] != '@'))
	if (_value[1] != '(' \|\| (_value[0] != '$' && _value[0] != '%' && _value[0] != '@') \|\| _value[_value.Length - 1] != ')')

		isLeftNum ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state),
		isLeftNum ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state));

		LeftChild.GetUnexpandedValue(state),
		LeftChild.GetExpandedValue(state),

		RightChild.GetUnexpandedValue(state),
		RightChild.GetExpandedValue(state),

Eval cond perf #6859

Eval cond perf #6859

Conversation

Forgind commented Sep 15, 2021 • edited Loading

Testing

Notes

Forgind commented Sep 17, 2021

Forgind Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

benvillalobos commented Sep 17, 2021

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

Forgind commented Oct 4, 2021 • edited Loading

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

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

Forgind commented Sep 15, 2021 •

edited

Loading

Forgind Sep 17, 2021 •

edited

Loading

Forgind commented Oct 4, 2021 •

edited

Loading