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
Andrew (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
Comment thread
iSazonov marked this conversation as resolved.
Outdated
Comment thread
iSazonov marked this conversation as resolved.
Outdated
private const char escapeChar = '`';

// Threshold for stack allocation.
// The size is less than MaxShortPath = 260.
private const int StackAllocThreshold = 256;
Comment thread
daxian-dbw marked this conversation as resolved.
Outdated

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

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 5, 2019

Copy link
Copy Markdown
Member

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.

@iSazonov Ilya (iSazonov) Jul 6, 2019

Copy link
Copy Markdown
Collaborator Author

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)
Comment thread
daxian-dbw marked this conversation as resolved.
Outdated
{
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)
Comment thread
daxian-dbw marked this conversation as resolved.
Outdated
{
s = new string(temp, 0, tempIndex);
s = pattern;

@TravisEz13 Travis Plunk (TravisEz13) Jul 2, 2019

Copy link
Copy Markdown
Member

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?

@iSazonov Ilya (iSazonov) Jul 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

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