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.

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

Optimize conversion of Key enum from/to string, reduce allocations #9697

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

h3xds1nz
Copy link
Contributor

@h3xds1nz h3xds1nz commented Sep 1, 2024

Description

Parsing optimization for Key and its conversion from/to string. Conversion from is now alloc-free, conversion to is either untouched or free.

  • ConvertFrom is now allocation free, optimizing performance from x4 for normal keys to x20 for F1-F12 keys.
    • The code-size of the method has increased but I think it is an acceptable compromise.
  • ConvertTo is pretty much unchanged however F10-F12 were added to switch as those are interned.
  • Adds/completes code documentation for the whole class.

ConvertFrom benchmarks (code size is not real here 'cause depth is set to 1)

Method source Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original \x20F5\x20 170.052 ns 1.7088 ns 1.5984 ns 0.0048 291 B 80 B
PR__EDIT \x20F5\x20 9.761 ns 0.0319 ns 0.0298 ns - 4,689 B -
Original F5 184.430 ns 3.5985 ns 3.3661 ns 0.0029 291 B 48 B
PR__EDIT F5 3.748 ns 0.0134 ns 0.0119 ns - 4,315 B -
Original \x20A\x20 22.271 ns 0.2355 ns 0.1967 ns 0.0043 291 B 72 B
PR__EDIT \x20A\x20 10.832 ns 0.0123 ns 0.0109 ns - 4,792 B -
Original Play 23.951 ns 0.3515 ns 0.3288 ns 0.0048 291 B 80 B
PR__EDIT Play 4.095 ns 0.0124 ns 0.0110 ns - 4,171 B -
Original A 16.815 ns 0.1958 ns 0.1832 ns 0.0029 291 B 48 B
PR__EDIT A 4.148 ns 0.0086 ns 0.0077 ns - 4,402 B -

ConvertTo benchmarks

Method source Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original Back 2.264 ns 0.0123 ns 0.0109 ns - 465 B -
PR__EDIT Back 2.252 ns 0.0123 ns 0.0109 ns - 545 B -
Original F10 14.026 ns 0.2135 ns 0.1892 ns 0.0014 473 B 24 B
PR__EDIT F10 2.460 ns 0.0082 ns 0.0068 ns - 567 B -
Original A 5.766 ns 0.1288 ns 0.1204 ns 0.0014 381 B 24 B
PR__EDIT A 5.257 ns 0.1345 ns 0.1439 ns 0.0014 553 B 24 B

Customer Impact

Increased performance, zero/reduced allocations.

Regression

No.

Testing

Local build, CI, assert tests:

// ConvertFrom Tests
Key x1 = (Key)oldKeyConverter.ConvertFrom(null, null, "F5");
Key x2 = (Key)newKeyConverter.ConvertFrom(null, null, "F5");
Assert.AreEqual(x1, x2);
x1 = (Key)oldKeyConverter.ConvertFrom(null, null, " BS");
x2 = (Key)newKeyConverter.ConvertFrom(null, null, " BS");
Assert.AreEqual(x1, x2);
x1 = (Key)oldKeyConverter.ConvertFrom(null, null, "A ");
x2 = (Key)newKeyConverter.ConvertFrom(null, null, "A ");
Assert.AreEqual(x1, x2);
x1 = (Key)oldKeyConverter.ConvertFrom(null, null, " Question ");
x2 = (Key)newKeyConverter.ConvertFrom(null, null, " Question ");
Assert.AreEqual(x1, x2);
x1 = (Key)oldKeyConverter.ConvertFrom(null, null, " OemQUEstion ");
x2 = (Key)newKeyConverter.ConvertFrom(null, null, " OemQUEstion ");
Assert.AreEqual(x1, x2);
x1 = (Key)oldKeyConverter.ConvertFrom(null, null, "  ");
x2 = (Key)newKeyConverter.ConvertFrom(null, null, "  ");
Assert.AreEqual(x1, x2);
// Invalid single character specified that is not a key
Assert.ThrowsException<ArgumentException>(() => oldKeyConverter.ConvertFrom(null, null, " á "));
Assert.ThrowsException<ArgumentException>(() => newKeyConverter.ConvertFrom(null, null, " á "));
// This is a case where 
// "NotSupportedException(SR.Format(SR.Unsupported_Key, fullName))"
// was expected but it is a dead code
Assert.ThrowsException<ArgumentException>(() => oldKeyConverter.ConvertFrom(null, null, " áb "));
Assert.ThrowsException<ArgumentException>(() => newKeyConverter.ConvertFrom(null, null, " áb "));

// CanConvertTo Tests
StandardContextImpl context = new StandardContextImpl();
context.Instance = (int)1;
bool x6 = oldKeyConverter.CanConvertTo(context, typeof(string));
bool x7 = newKeyConverter.CanConvertTo(context, typeof(string));
Assert.AreEqual(x6, x7);
context.Instance = (Key)1;
x6 = oldKeyConverter.CanConvertTo(context, typeof(string));
x7 = newKeyConverter.CanConvertTo(context, typeof(string));
Assert.AreEqual(x6, x7);
context.Instance = (byte)1;
Assert.ThrowsException<InvalidCastException>(() => oldKeyConverter.CanConvertTo(context, typeof(string)));
Assert.ThrowsException<InvalidCastException>(() => newKeyConverter.CanConvertTo(context, typeof(string)));

// ConvertTo Tests
string x3 = (string)oldKeyConverter.ConvertTo(null, null, Key.A, typeof(string));
string x4 = (string)newKeyConverter.ConvertTo(null, null, Key.A, typeof(string));
Assert.AreEqual(x3, x4);

x3 = (string)oldKeyConverter.ConvertTo(null, null, Key.F10, typeof(string));
x4 = (string)newKeyConverter.ConvertTo(null, null, Key.F10, typeof(string));
Assert.AreEqual(x3, x4);

x3 = (string)oldKeyConverter.ConvertTo(null, null, Key.Back, typeof(string));
x4 = (string)newKeyConverter.ConvertTo(null, null, Key.Back, typeof(string));
Assert.AreEqual(x3, x4);

x3 = (string)oldKeyConverter.ConvertTo(null, null, Key.DeadCharProcessed, typeof(string));
x4 = (string)newKeyConverter.ConvertTo(null, null, Key.DeadCharProcessed, typeof(string));
Assert.AreEqual(x3, x4);

x3 = (string)oldKeyConverter.ConvertTo(null, null, Key.None, typeof(string));
x4 = (string)newKeyConverter.ConvertTo(null, null, Key.None, typeof(string));
Assert.AreEqual(x3, x4);

//Null destinationType
Assert.ThrowsException<ArgumentNullException>(() => (string)oldKeyConverter.ConvertTo(null, null, (Key)(-1), null));
Assert.ThrowsException<ArgumentNullException>(() => (string)newKeyConverter.ConvertTo(null, null, (Key)(-1), null));
//Invalid destinationType
Assert.ThrowsException<NotSupportedException>(() => (string)oldKeyConverter.ConvertTo(null, null, (Key)(-1), typeof(Int32)));
Assert.ThrowsException<NotSupportedException>(() => (string)newKeyConverter.ConvertTo(null, null, (Key)(-1), typeof(Int32)));
//Value out of range
Assert.ThrowsException<NotSupportedException>(() => (string)oldKeyConverter.ConvertTo(null, null, (Key)(-1), typeof(string)));
Assert.ThrowsException<NotSupportedException>(() => (string)newKeyConverter.ConvertTo(null, null, (Key)(-1), typeof(string)));
//Value is null
Assert.ThrowsException<NotSupportedException>(() => (string)oldKeyConverter.ConvertTo(null, null, null, typeof(string)));
Assert.ThrowsException<NotSupportedException>(() => (string)newKeyConverter.ConvertTo(null, null, null, typeof(string)));

Risk

Low, changes have been thoroughly tested.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners September 1, 2024 12:31
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions Included in test pass PR metadata: Label to tag PRs, to facilitate with triage Status:Completed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants