Skip to content

feat: Improve ToString performance for Core.Mathematics structs #2788

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

Merged
merged 5 commits into from
May 24, 2025

Conversation

Kryptos-FR
Copy link
Member

@Kryptos-FR Kryptos-FR commented May 15, 2025

PR Details

Implements ISpanFormattable for most types in Core.Mathematics, inspired by this article High-performance string formatting in .NET and related reddit post.

As a side effect, a few ToString() implementations were fixed (which would have generated wrong values when given a custom format).

Also, there are now only two ToString methods:

  • ToString() overridden from object
  • ToString(string?, IFormatProvider?) implementing IFormattable
  • ToString(string?) and ToString(IFormatProvider?) can be simulated by passing null for the other argument.

Benchmarks

The gain is not significant for most types (but always positive). It is more visible for Matrix which has a lot of fields.

Matrix (before)

Method Mean Ratio Allocated Alloc Ratio
ToString 859.2 ns 1.00 768 B 1.00
ToString(string?) 1,312.2 ns 1.53 960 B 1.25
ToString(IFormatProvider?) 987.5 ns 1.15 768 B 1.00
ToString(string?, IFormatProvider?) 1,000.7 ns 1.16 792 B 1.03

Matrix (after)

Method Mean Ratio Allocated Alloc Ratio
ToString 678.0 ns 1.00 232 B 1.00
ToString(string?) 997.9 ns 1.47 376 B 1.62
ToString(IFormatProvider?) 666.4 ns 0.98 312 B 1.34
ToString(string?, IFormatProvider?) 997.6 ns 1.47 376 B 1.62

Side by side comparison

Method Mean Ratio Allocated Alloc Ratio
ToString 859.2 ns 1.00 768 B 1.00
ToString 678.0 ns 0.79 232 B 0.30
ToString(string?) 1,312.2 ns 1.00 960 B 1.00
ToString(string?) 997.9 ns 0.76 376 B 0.39
ToString(IFormatProvider?) 987.5 ns 1.00 768 B 1.00
ToString(IFormatProvider?) 666.4 ns 0.67 312 B 0.41
ToString(string?, IFormatProvider?) 1,000.7 ns 1.00 792 B 1.00
ToString(string?, IFormatProvider?) 997.6 ns 1.00 376 B 0.47

Note: in these benchmarks the format "0.##" and the provider CultureInfo.CurrentCulture are passed as parameters which explains why the benchmark for ToString(string?, IFormatProvider?) is slightly worse than ToString(IFormatProvider?) since the custom format causes more computation and allocations. If both are null the performance is similar to ToString().

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@Kryptos-FR Kryptos-FR self-assigned this May 15, 2025
@Kryptos-FR Kryptos-FR added enhancement New feature or request bug-fix labels May 15, 2025
@Kryptos-FR
Copy link
Member Author

@Eideren do you think I should add unit tests to check the result of calling ToString() for these types?

@Kryptos-FR Kryptos-FR force-pushed the feature/tostring-performance branch from 64a28c5 to b4e033b Compare May 15, 2025 16:19
@Eideren
Copy link
Collaborator

Eideren commented May 15, 2025

@Kryptos-FR maybe just for round-trippable values ?

@Kryptos-FR
Copy link
Member Author

Kryptos-FR commented May 16, 2025

@Eideren for round-trips there are already these tests: https://github.com/stride3d/stride/blob/6183210a2ba67f82fe3c3321fcf8ba89009e31d1/sources/core/Stride.Core.Design.Tests/TestTypeConverter.cs and they pass.

I was more thinking of adding test to guarantee the format of the string doesn't change over time and if so we have a test to think about it. I think I'll just do that.

edit: tests added.

@Kryptos-FR Kryptos-FR force-pushed the feature/tostring-performance branch from 4060f20 to ca08d6b Compare May 16, 2025 12:38
@Kryptos-FR Kryptos-FR requested a review from Eideren May 16, 2025 12:43
@Kryptos-FR Kryptos-FR force-pushed the feature/tostring-performance branch from ca08d6b to a9e9589 Compare May 16, 2025 12:52
Copy link
Collaborator

@Eideren Eideren left a comment

Choose a reason for hiding this comment

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

Looks good, just a small note

@@ -1532,7 +1532,6 @@ Global
{6F473FA6-4F8B-4FBA-AE33-EE5AF997D50C} = {1AE1AC60-5D2F-4CA7-AE20-888F44551185}
{4A15BAAD-AA37-4754-A2BF-8D4049211E36} = {1AE1AC60-5D2F-4CA7-AE20-888F44551185}
{1AC70118-C90F-4EC6-9D8B-C628BDF900F7} = {4C142567-C42B-40F5-B092-798882190209}
{B175D318-B4D0-49EA-9AEF-A54ACA2F03DC} = {25F10A0B-7259-404C-86BE-FD2363C92F72}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this one for ?

Copy link
Member Author

Choose a reason for hiding this comment

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

One test project was mistakenly move to a solution folder in a previous PR. I restored its location in the solution explorer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would have been better as a separate PR, but doesn't matter too much, feel free to merge when you are ready

@Kryptos-FR Kryptos-FR merged commit 0285db7 into stride3d:master May 24, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants