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.

Sign up for GitHub

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

Jump to bottom

Eval cond perf #6859

Merged
merged 10 commits into from
Oct 26, 2021
Merged

Eval cond perf #6859

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e02b3d9
Early exit
Forgind Sep 13, 2021
64354ff
Prevent double computation
Forgind Sep 13, 2021
762ac1d
Use search for @
Forgind Sep 14, 2021
f9456c4
Fix incorrect bool check
Forgind Sep 22, 2021
16a30b3
Throw error
Forgind Oct 4, 2021
6e4c0d4
Replace Can*Evaluate and *Evaluate with Try*Evaluate
Forgind Oct 11, 2021
2aac6e2
Do not abbreviate variable name
Forgind Oct 11, 2021
0134385
Switch switch to tuple
Forgind Oct 11, 2021
625fd3f
Switch order of check
Forgind Oct 11, 2021
62760c1
Throw exception where necessary
Forgind Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/Build/Evaluation/Conditionals/StringExpressionNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Diagnostics;

using Microsoft.Build.Shared;

namespace Microsoft.Build.Evaluation
Expand Down Expand Up @@ -98,6 +97,25 @@ internal override bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationS
{
if (_expandable)
{
switch (_value.Length)
{
case 0:
_cachedExpandedValue = String.Empty;
return true;
// If the length is 1 or 2, it can't possibly be a property, item, or metadata, and it isn't empty.
case 1:
case 2:
_cachedExpandedValue = _value;
return false;
default:
if (_value[1] != '(' || _value[_value.Length - 1] != ')' || (_value[0] != '$' && _value[0] != '%' && _value[0] != '@'))
Copy link
Member

Choose a reason for hiding this comment

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

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

{
// This isn't just a property, item, or metadata value, and it isn't empty.
return false;
}
break;
}

string expandBreakEarly = state.ExpandIntoStringBreakEarly(_value);

if (expandBreakEarly == null)
Expand Down