-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: Improve ToString performance for Core.Mathematics structs #2788
Conversation
@Eideren do you think I should add unit tests to check the result of calling |
64a28c5
to
b4e033b
Compare
@Kryptos-FR maybe just for round-trippable values ? |
@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. |
4060f20
to
ca08d6b
Compare
See Stride.Core.Assets.Editor.SupportedLanguage for the list
ca08d6b
to
a9e9589
Compare
There was a problem hiding this 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} |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
PR Details
Implements
ISpanFormattable
for most types inCore.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 fromobject
ToString(string?, IFormatProvider?)
implementingIFormattable
ToString(string?)
andToString(IFormatProvider?)
can be simulated by passingnull
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)
Matrix (after)
Side by side comparison
Note: in these benchmarks the format
"0.##"
and the providerCultureInfo.CurrentCulture
are passed as parameters which explains why the benchmark forToString(string?, IFormatProvider?)
is slightly worse thanToString(IFormatProvider?)
since the custom format causes more computation and allocations. If both arenull
the performance is similar toToString()
.Types of changes
Checklist