Skip to content

Optimize conversion of ModifierKeys enum from/to string, reduce allocations#9683

Merged
harshit7962 merged 8 commits into
dotnet:mainfrom
h3xds1nz:modifierkeys-speedup
Jan 3, 2025
Merged

Optimize conversion of ModifierKeys enum from/to string, reduce allocations#9683
harshit7962 merged 8 commits into
dotnet:mainfrom
h3xds1nz:modifierkeys-speedup

Conversation

@h3xds1nz
Copy link
Copy Markdown
Member

@h3xds1nz h3xds1nz commented Aug 30, 2024

Fixes #7502

Description

Parsing optimization for ModifierKeys and its conversion from/to string. Conversion from is now alloc-free, conversion to is either free or reduced by a big margin (see benchmarks).

  • ConvertFrom is now fully flying on ReadOnlySpan<char> instead of multiple substrings and trims on string instances.
  • ConvertTo is now within 2 procs, fast-path and slow-path. The only allocated string instance is with multiple modifiers.
  • Strings in ConvertTo will now be interned with the rest since we use StringComparison.OrdinalIgnoreCase.
  • ModifierKeysFlag static field is now a compile-time constant, saving memory and providing speed-up.
  • The JITTed codegen takes up less memory as we removed few things.
  • Adds/completes code documentation for the whole class.

ConvertFrom Benchmark for all possible modifiers

Method source Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size Allocated
Original CTRL (...)+ ALT [24] 101.55 ns 1.790 ns 1.587 ns 0.0224 2,662 B 376 B
PR__EDIT CTRL (...)+ ALT [24] 34.68 ns 0.218 ns 0.182 ns - 1,891 B -
Original Ctrl (...)+ Alt [28] 147.59 ns 1.616 ns 1.512 ns 0.0319 2,640 B 536 B
PR__EDIT Ctrl (...)+ Alt [28] 35.19 ns 0.483 ns 0.451 ns - 1,891 B -

ConvertTo benchmark for different scenarios

Method value Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original None 1.0984 ns 0.0108 ns 0.0090 ns - 685 B -
PR__EDIT None 0.8665 ns 0.0078 ns 0.0066 ns - 238 B -
Original Shift 1.1785 ns 0.0149 ns 0.0132 ns - 780 B -
PR__EDIT Shift 0.8840 ns 0.0209 ns 0.0196 ns - 255 B -
Original 3 10.517 ns 0.1322 ns 0.1104 ns 0.0043 1,134 B 72 B
PR__EDIT 3 9.546 ns 0.1713 ns 0.1518 ns 0.0024 756 B 40 B
Original 15 39.23 ns 0.828 ns 0.886 ns 0.0176 1,137 B 296 B
PR__EDIT 15 11.88 ns 0.194 ns 0.181 ns 0.0043 753 B 72 B

Customer Impact

Increased performance, zero/reduced allocations, less static memory.

Regression

No.

Testing

Local build, CI, assert tests:

//ConvertTo
string x1 = Original(ModifierKeys.None);
string x2 = PR__EDIT(ModifierKeys.None);
Assert.AreEqual(x1, x2);
x1 = Original(ModifierKeys.Shift);
x2 = PR__EDIT(ModifierKeys.Shift);
Assert.AreEqual(x1, x2);
x1 = Original(ModifierKeys.Shift | ModifierKeys.Alt);
x2 = PR__EDIT(ModifierKeys.Shift | ModifierKeys.Alt);
Assert.AreEqual(x1, x2);
x1 = Original(ModifierKeys.Control | ModifierKeys.Alt);
x2 = PR__EDIT(ModifierKeys.Control | ModifierKeys.Alt);
Assert.AreEqual(x1, x2);
x1 = Original(ModifierKeys.Windows | ModifierKeys.Alt);
x2 = PR__EDIT(ModifierKeys.Windows | ModifierKeys.Alt);
Assert.AreEqual(x1, x2);
x1 = Original(ModifierKeys.Windows | ModifierKeys.Alt | ModifierKeys.Control | ModifierKeys.Shift);
x2 = Original(ModifierKeys.Windows | ModifierKeys.Alt | ModifierKeys.Control | ModifierKeys.Shift);
Assert.AreEqual(x1, x2);

//ConvertFrom
ModifierKeys x1 = Original("CTRL + ");
ModifierKeys x2 = PR__EDIT("CTRL + ");
Assert.AreEqual(x1, x2);
x1 = Original("CTRL");
x2 = PR__EDIT("CTRL");
Assert.AreEqual(x1, x2);
x1 = Original(" ALT + SHIFT ");
x2 = PR__EDIT(" ALT + SHIFT ");
Assert.AreEqual(x1, x2);
x1 = Original("WIN + WINDOWS");
x2 = PR__EDIT("WIN + WINDOWS");
Assert.AreEqual(x1, x2);
x1 = Original("   ");
x2 = PR__EDIT("   ");
Assert.AreEqual(x1, x2);
x1 = Original("CTRL + WIN + SHIFT + ALT");
x2 = PR__EDIT("CTRL + WIN + SHIFT + ALT");
Assert.AreEqual(x1, x2);
x1 = Original("CTRL ++ WIN");
x2 = PR__EDIT("CTRL ++ WIN");
Assert.AreEqual(x1, x2);
x1 = Original("ALT+WIN");
x2 = PR__EDIT("ALT+WIN");
Assert.AreEqual(x1, x2);

Risk

Low.

Microsoft Reviewers: Open in CodeFlow

harshit7962
harshit7962 previously approved these changes Jan 3, 2025
Copy link
Copy Markdown
Member

@harshit7962 harshit7962 left a comment

Choose a reason for hiding this comment

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

@h3xds1nz, the PR can be taken in after resolution of the conflict.

@harshit7962
Copy link
Copy Markdown
Member

Also, as we already have WindowsBase project already set-up for unit tests, can we add some tests for this change?

@h3xds1nz
Copy link
Copy Markdown
Member Author

h3xds1nz commented Jan 3, 2025

@harshit7962 I didn't PR any as there are tests in #8215, same for KeyConverter. I've unfortunately found out after I wrote them myself, haha.

On my way with the conflicts.

Copy link
Copy Markdown
Member

@harshit7962 harshit7962 left a comment

Choose a reason for hiding this comment

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

Reapproving post resolution of conflict.

@harshit7962 harshit7962 merged commit 14c9db3 into dotnet:main Jan 3, 2025
@harshit7962
Copy link
Copy Markdown
Member

Thanks for the contribution @h3xds1nz.

@h3xds1nz
Copy link
Copy Markdown
Member Author

h3xds1nz commented Jan 3, 2025

@harshit7962 Thanks for taking it :)

@github-actions github-actions Bot locked and limited conversation to collaborators Feb 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Windows.Input.ModifierKeysConverter.GetModifierKeys has unnecessary allocations

2 participants