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

Remove allocations on all base converters, improve TokenizerHelper #9364

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

Conversation

h3xds1nz
Copy link
Member

@h3xds1nz h3xds1nz commented Jul 7, 2024

Description

This is probably one of the impactful PRs I've posted; optimizes conversion/parsing of all the base-generated types but also ThicknessConverter (once more), CornerRadiusConverter, KeySplineConverter and VirtualizationCacheLengthConverter.

Completely removes any allocations caused by the original Tokenizer by using ReadOnlySpan<char> instead of substrings.
I have also introduced a ref struct variant that can be used in any code that doesn't compile for net472.

MilCodeGen has also been adjusted to support this.

Single Thickness parsing (4 units)

Method Mean [ns] Error [ns] StdDev [ns] Code Size [B] Gen0 Allocated [B]
Original 237.4 ns 4.64 ns 4.34 ns 555 B 0.0105 176 B
PR_EDITREF 191.6 ns 1.62 ns 1.51 ns 761 B - -

Parsing Thickness 10 times (4 units)

Method Mean [ns] Error [ns] StdDev [ns] Code Size [B] Gen0 Allocated [B]
Original 2,434.0 ns 17.02 ns 14.08 ns 608 B 0.1030 1760 B
PR_EDITREF 1,885.1 ns 5.68 ns 4.44 ns 814 B - -

Creation of 10x Matrix3D via Parse

Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 7,643.3 ns 26.76 ns 20.89 ns 0.3052 6,876 B 5200 B
PR_EDIT 5,885.4 ns 67.98 ns 60.27 ns - 7,177 B -

Customer Impact

Improved performance by around 20% on base types, zeroed out allocations.

Regression

No.

Testing

Local build, basic testing of the tokenizer with span and some of the converters.

Risk

Low, the scope of the PR is big but with a simple change, hence any mistakes shall be easy to figure out on a DRT run.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners July 7, 2024 12:45
@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 LengthConverter to minimum and improve conversion performance Reduce LengthConverter allocs to minimum and improve conversion performance Jul 7, 2024
@h3xds1nz h3xds1nz force-pushed the remove-allocs-on-lengthconverter branch 3 times, most recently from 4a84838 to 15380c3 Compare August 27, 2024 17:28
@h3xds1nz h3xds1nz marked this pull request as draft August 27, 2024 18:40
@h3xds1nz h3xds1nz changed the title Reduce LengthConverter allocs to minimum and improve conversion performance Remove allocations on all base converters, improve TokenizerHelper Oct 3, 2024
@h3xds1nz h3xds1nz force-pushed the remove-allocs-on-lengthconverter branch from 15380c3 to beced18 Compare October 3, 2024 17:04
@h3xds1nz h3xds1nz marked this pull request as ready for review October 3, 2024 17:05
@h3xds1nz h3xds1nz force-pushed the remove-allocs-on-lengthconverter branch from 4cb1aca to aadcc3b Compare October 7, 2024 16:47
@h3xds1nz
Copy link
Member Author

h3xds1nz commented Oct 7, 2024

Resolved merge conflicts 12, there are better ways to spent an hour and a half.

Copy link
Contributor

@ThomasGoulet73 ThomasGoulet73 left a comment

Choose a reason for hiding this comment

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

Side note: @h3xds1nz when there's a review on a PR I recommend adding new commits and doing merges instead of rebasing. It makes it much easier to track what changed since the last review.


PixelUnit pixelUnit;
if (PixelUnit.TryParsePixel(valueSpan, out pixelUnit)
|| PixelUnit.TryParsePixelPerInch(valueSpan, out pixelUnit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the || or && at the start of the line instead of at the end of the line wrong according to the coding style ? Both seems to be used a lot and if one isn't better than the other I don't see a reason to change it.

if (valueSpan.Equals("auto", StringComparison.OrdinalIgnoreCase))
return Double.NaN;

PixelUnit pixelUnit;
Copy link
Contributor

Choose a reason for hiding this comment

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

When I wrote this I put it on its own line on purpose because I think it's cleaner when the variable is used multiple times. Is there a reason you changed it ? I know there's an IDE0018 suggestion to change it but projects like winforms or runtime keep it as a suggestion and reason might be that sometimes it's cleaner not to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will revert the stylistic changes to your code before merge if you think it's cleaner, as it is your code. I just auto formatted here (which does for me a tad more than pure .editorconfig).

@himgoyalmicro
Copy link
Contributor

Hey @h3xds1nz,
Sorry for the delay in response. We have observed that when we do
Rect testRect = Rect.Parse(" Empty \t");
We are hitting the exception System.FormatException: 'The input string 'Empty' was not in correct format.'

@h3xds1nz h3xds1nz force-pushed the remove-allocs-on-lengthconverter branch from aadcc3b to 270db60 Compare April 1, 2025 09:27
@h3xds1nz
Copy link
Member Author

h3xds1nz commented Apr 1, 2025

@himgoyalmicro Right, that was silly. Fixed.

I had to include #10683 for now to be able to run MilCodeGen, that's a PR that doesn't really need tests, could be just approved.

Copy link

codecov bot commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 45.36341% with 218 lines in your changes missing coverage. Please review.

Project coverage is 11.41570%. Comparing base (3cc9d57) to head (8b733a1).
Report is 15 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main       #9364         +/-   ##
===================================================
+ Coverage   11.17950%   11.41570%   +0.23619%     
===================================================
  Files           3314        3316          +2     
  Lines         665182      665312        +130     
  Branches       74668       74690         +22     
===================================================
+ Hits           74364       75950       +1586     
+ Misses        589525      587950       -1575     
- Partials        1293        1412        +119     
Flag Coverage Δ
Debug 11.41570% <45.36341%> (+0.23619%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

h3xds1nz added a commit to h3xds1nz/wpf that referenced this pull request Apr 5, 2025
@h3xds1nz h3xds1nz force-pushed the remove-allocs-on-lengthconverter branch from d0a827e to 3c551cb Compare April 5, 2025 09:32
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:InProgress Status:Proposed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants