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

Reduce ThicknessConverter allocs to minimum and improve conversion performance #9363

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

Conversation

h3xds1nz
Copy link

@h3xds1nz h3xds1nz commented Jul 7, 2024

Description

Simplifies code in CanConvertTo/ConvertFrom/ConvertTo, removes additional type casts by using is checks/pattern match. Commits should be easy to review.

Uses DefaultInterpolatedStringHandler instead of StringBuilder to handle conversion in ToString with a pre-allocated on-stack buffer (not using interpolated strings directly due to the NaN->Auto conversion which would box as Roslyn won't handle that one.

Also stackallocs 4 doubles instead of allocating an array in FromString, this uses unsafe quotation due to how the current code-gen with Span looks like; additional perf improvement. I didn't include benchmark/tests in this one, they're together in the other PR for LengthConverter #9364.

Multiple decimals point (edge case but within original range)

ToString(new Thickness(3.33371, 2.25888, 6.6666, 8.444), CultureInfo.InvariantCulture);
Method Mean [ns] Error [ns] StdDev [ns] Gen0 Allocated [B]
PR_EDIT 348.2 ns 2.56 ns 2.27 ns 0.0048 80 B
Original 393.2 ns 3.05 ns 2.70 ns 0.0257 432 B

Single decimal point, standard use-case

ToString(new Thickness(1, 2, 2.5, 4), CultureInfo.InvariantCulture);
Method Mean [ns] Error [ns] StdDev [ns] Gen0 Allocated [B]
PR_EDIT 277.4 ns 2.22 ns 2.08 ns 0.0024 40 B
Original 328.1 ns 4.37 ns 4.09 ns 0.0205 344 B

Conversion from double.NaN to Auto

ToString(new Thickness(double.NaN), CultureInfo.InvariantCulture);
Method Mean [ns] Error [ns] StdDev [ns] Gen0 Allocated [B]
PR_EDIT 24.23 ns 0.187 ns 0.146 ns 0.0038 64 B
Original 32.90 ns 0.441 ns 0.391 ns 0.0157 264 B

Customer Impact

Improved performance, greatly decreased allocations.

Regression

No.

Testing

Local build, basic set of assert testing.

CultureInfo CachedNorwegian = new CultureInfo("nn-NO");

Assert.AreEqual(ToString(new Thickness(1.5, 2, 1.25, 3), CultureInfo.InvariantCulture), ToString2(new Thickness(1.5, 2, 1.25, 3), CultureInfo.InvariantCulture));
Assert.AreEqual(ToString(new Thickness(3, 2, 6, 8), CultureInfo.InvariantCulture), ToString2(new Thickness(3, 2, 6, 8), CultureInfo.InvariantCulture));
Assert.AreEqual(ToString(new Thickness(3.33371, 2.25888, 6.6666, 8.444), CultureInfo.InvariantCulture), ToString2(new Thickness(3.33371, 2.25888, 6.6666, 8.444), CultureInfo.InvariantCulture));

//Culture with decimal comma separator
Assert.AreEqual(ToString(new Thickness(1.5, 2, 1.25, 3), CachedNorwegian), ToString2(new Thickness(1.5, 2, 1.25, 3), CachedNorwegian));
Assert.AreEqual(ToString(new Thickness(3, 2, 6, 8), CachedNorwegian), ToString2(new Thickness(3, 2, 6, 8), CachedNorwegian));
Assert.AreEqual(ToString(new Thickness(3.33371, 2.25888, 6.6666, 8.444), CachedNorwegian), ToString2(new Thickness(3.33371, 2.25888, 6.6666, 8.444), CachedNorwegian));

Assert.AreEqual(ToString(new Thickness(), CachedNorwegian), ToString2(new Thickness(), CachedNorwegian));

Assert.AreEqual(ToString(new Thickness(double.NaN), CachedNorwegian), ToString2(new Thickness(double.NaN), CachedNorwegian));

Risk

Low.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners July 7, 2024 11:46
@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 Jul 7, 2024
@h3xds1nz h3xds1nz changed the title Reduce allocs ThicknessConverter to minimum and improve conversion performance Reduce ThicknessConverter allocs to minimum and improve conversion performance Jul 7, 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 PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants