Skip to content

Reduce ThicknessConverter allocs to minimum and improve conversion performance #9363

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

Jump to bottom
Merged
merged 5 commits into from
Oct 3, 2024
Merged

Reduce ThicknessConverter allocs to minimum and improve conversion performance #9363

merged 5 commits into from
Oct 3, 2024

Conversation

h3xds1nz
Copy link
Member

@h3xds1nz h3xds1nz commented Jul 7, 2024 •
edited
Loading

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 mentioned this pull request Jul 7, 2024
Open
@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
@harshit7962
Copy link
Member

/azp run

1 similar comment
@dipeshmsft
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@harshit7962
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

harshit7962
harshit7962 previously approved these changes Sep 9, 2024
View reviewed changes
@h3xds1nz h3xds1nz mentioned this pull request Sep 23, 2024
Merged
@h3xds1nz h3xds1nz force-pushed the remove-allocs-on-thickness-converter branch from 2dd4be2 to 72058ef Compare October 1, 2024 13:15
@h3xds1nz
Copy link
Member Author

h3xds1nz commented Oct 1, 2024

@harshit7962 I have fixed the merge conflicts and rebased. No other changes have been done.

harshit7962
harshit7962 approved these changes Oct 3, 2024
View reviewed changes
Copy link
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.

Re-Approving after a stale review. Thank you @h3xds1nz for resolving the conflicts

@harshit7962 harshit7962 merged commit 673c354 into dotnet:main Oct 3, 2024
8 checks passed
@harshit7962
Copy link
Member

Thank you @h3xds1nz for your contributions.

@h3xds1nz
Copy link
Member Author

h3xds1nz commented Oct 3, 2024 •
edited
Loading

@harshit7962 Thank you for making this one happen, sent #9364 in and marked it as ready. That will clear out rest of the allocations.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@harshit7962 harshit7962 harshit7962 approved these changes

@dipeshmsft dipeshmsft Awaiting requested review from dipeshmsft

@singhashish-wpf singhashish-wpf Awaiting requested review from singhashish-wpf

+1 more reviewer

@ThomasGoulet73 ThomasGoulet73 ThomasGoulet73 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@h3xds1nz h3xds1nz

Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@h3xds1nz @harshit7962 @dipeshmsft @ThomasGoulet73