Skip to content

Reduce allocations in Escape() and Unescape() #10041

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

Merged
anmenaga merged 6 commits into from
Jul 19, 2019
Merged

Reduce allocations in Escape() and Unescape() #10041

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6cc75c8
Reduce allocations in Escape() and Unescape()
iSazonov Jul 1, 2019
4d7c316
Use nameof()
iSazonov Jul 1, 2019
ee3a005
Fix CodeFactor issue
iSazonov Jul 1, 2019
68c349a
Address feedback
iSazonov Jul 3, 2019
b409065
Fix CodeFactor issue
iSazonov Jul 4, 2019
b4917fc
Add condition for empty pattern
iSazonov Jul 6, 2019
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
37 changes: 26 additions & 11 deletions src/System.Management.Automation/engine/regex.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ public sealed class WildcardPattern
// char that escapes special chars
private const char escapeChar = '`';

// Threshold for stack allocation.
// The size is less than MaxShortPath = 260.
private const int StackAllocThreshold = 256;

// we convert a wildcard pattern to a predicate
private Predicate<string> _isMatch;

Expand Down Expand Up @@ -203,15 +207,20 @@ internal static string Escape(string pattern, char[] charsNotToEscape)
{
if (pattern == null)
{
throw PSTraceSource.NewArgumentNullException("pattern");
throw PSTraceSource.NewArgumentNullException(nameof(pattern));
}

if (charsNotToEscape == null)
{
throw PSTraceSource.NewArgumentNullException("charsNotToEscape");
throw PSTraceSource.NewArgumentNullException(nameof(charsNotToEscape));
}

if (pattern == string.Empty)
{
return pattern;
}

char[] temp = new char[pattern.Length * 2 + 1];
Span<char> temp = pattern.Length < StackAllocThreshold ? stackalloc char[pattern.Length * 2 + 1] : new char[pattern.Length * 2 + 1];
Copy link
Member

@daxian-dbw daxian-dbw Jul 5, 2019

Choose a reason for hiding this comment

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

This is a bit misleading. StackAllocThreshold is used as the upper limit for the buffer to be allocated elsewhere. Here it's not the buffer size limit, but the pattern length limit.

Copy link
Collaborator Author

@iSazonov iSazonov Jul 6, 2019

Choose a reason for hiding this comment

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

Perhaps, but other options look even worse (I mean renaming and using 2 constants). On the other hand, in both cases we can consider this condition just as a boundary based on the length of the pattern.

int tempIndex = 0;

for (int i = 0; i < pattern.Length; i++)
Expand All @@ -231,13 +240,13 @@ internal static string Escape(string pattern, char[] charsNotToEscape)

string s = null;

if (tempIndex > 0)
if (tempIndex == pattern.Length)
{
s = new string(temp, 0, tempIndex);
s = pattern;
}
else
{
s = string.Empty;
s = new string(temp.Slice(0, tempIndex));
}

return s;
Expand Down Expand Up @@ -312,10 +321,16 @@ public static string Unescape(string pattern)
{
if (pattern == null)
{
throw PSTraceSource.NewArgumentNullException("pattern");
throw PSTraceSource.NewArgumentNullException(nameof(pattern));
}

char[] temp = new char[pattern.Length];
if (pattern == string.Empty)
{
return pattern;
}

Span<char> temp = pattern.Length < StackAllocThreshold ? stackalloc char[pattern.Length] : new char[pattern.Length];

int tempIndex = 0;
bool prevCharWasEscapeChar = false;

Expand Down Expand Up @@ -361,13 +376,13 @@ public static string Unescape(string pattern)

string s = null;

if (tempIndex > 0)
if (tempIndex == pattern.Length)
{
s = new string(temp, 0, tempIndex);
s = pattern;
Copy link
Member

@TravisEz13 TravisEz13 Jul 2, 2019

Choose a reason for hiding this comment

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

Do we have tests cases for regex escape?

Copy link
Collaborator Author

@iSazonov iSazonov Jul 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Only indirectly - the API is used in engine and involved in paths, completions and other tests.
I open new tracking issue #10051 to add tests for WildcardPattern class.

Update:

PowerShell/src/System.Management.Automation/engine/regex.cs

Line 722 in 0d55a72

/// for a more cases see the unit-test file RegexTest.cs

Perhaps MSFT team can port the Unit tests.

}
else
{
s = string.Empty;
s = new string(temp.Slice(0, tempIndex));
}

return s;
Expand Down