Skip to content

use MathF where possible#19965

Merged
PureWeen merged 8 commits into
dotnet:mainfrom
symbiogenesis:mathf
Jul 11, 2024
Merged

use MathF where possible#19965
PureWeen merged 8 commits into
dotnet:mainfrom
symbiogenesis:mathf

Conversation

@symbiogenesis
Copy link
Copy Markdown
Contributor

When doing floating point math, we should be using MathF in order to do the calculations as native floating point math. Thus reducing casting and improving performance.

@symbiogenesis symbiogenesis requested a review from a team as a code owner January 18, 2024 08:41
@ghost ghost added the community ✨ Community Contribution label Jan 18, 2024
@ghost
Copy link
Copy Markdown

ghost commented Jan 18, 2024

Hey there @symbiogenesis! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@jsuarezruiz
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow
mattleibow previously approved these changes Jan 19, 2024
@mattleibow
Copy link
Copy Markdown
Member

/azp run

@mattleibow mattleibow enabled auto-merge (squash) January 19, 2024 07:54
@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 3 pipeline(s).

@symbiogenesis
Copy link
Copy Markdown
Contributor Author

Ok, so this is failing on .NET Standard 2.0 because MathF only exists in .NET Standard 2.1 and above.

Is NETSTANDARD2_0 going to be supported on main much longer? If not, then this could wait until that point.

@jonathanpeppers
Copy link
Copy Markdown
Member

We will need NETSTANDARD2_0 until Visual Studio moves off .NET Framework. This could be years, no one actually knows.

@symbiogenesis
Copy link
Copy Markdown
Contributor Author

symbiogenesis commented Jan 19, 2024

One option would be to alias MathF on NETSTANDARD2_0 to an internal class. Or just add it directly under the System namespace, while keeping it internal.

auto-merge was automatically disabled January 19, 2024 19:30

Head branch was pushed to by a user without write access

@symbiogenesis symbiogenesis force-pushed the mathf branch 6 times, most recently from 61033af to c2125b1 Compare January 20, 2024 16:49
Comment thread src/Graphics/src/Graphics/MathF.cs
Comment thread src/Core/src/Primitives/MathF.cs Outdated
@symbiogenesis
Copy link
Copy Markdown
Contributor Author

symbiogenesis commented Apr 7, 2024

I think you missed one?

@jonathanpeppers fixed

@jonathanpeppers
Copy link
Copy Markdown
Member

/azp run MAUI-UITests-public

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@symbiogenesis symbiogenesis requested a review from mattleibow April 8, 2024 17:16
Copy link
Copy Markdown
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

It would be nice if this one had a Benchmark to know how much it helps. But it seems plausible this should help -- also no new public APIs, straightforward, etc.

@symbiogenesis
Copy link
Copy Markdown
Contributor Author

symbiogenesis commented Apr 21, 2024

Added a PathBenchmarker. 2.6% improvement. No big win, but it makes sense for low level code to be optimized, and allow people to build towers of abstraction upon it.

If someone wanted to build games on MAUI, for example, then it might matter more. Building thousands of paths in that context may make sense.

And, of course, now MathF will be available for further uses.

Before

Method Mean Error StdDev Gen0 Gen1 Allocated
BuildPath 27.99 us 0.546 us 1.359 us 4.3335 0.2136 40.08 KB

After

Method Mean Error StdDev Gen0 Gen1 Allocated
BuildPath 27.27 us 0.341 us 0.791 us 4.3335 0.1831 40.08 KB

@jonathanpeppers
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 3 pipeline(s).

@Eilon Eilon added the perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label May 10, 2024
@PureWeen PureWeen added the area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing label May 31, 2024
@PureWeen PureWeen merged commit 67914ef into dotnet:main Jul 11, 2024
@symbiogenesis symbiogenesis deleted the mathf branch July 11, 2024 01:14
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Aug 2, 2024
@samhouts samhouts added fixed-in-8.0.80 fixed-in-net9.0-nightly This may be available in a nightly release! and removed fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Aug 8, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing community ✨ Community Contribution fixed-in-8.0.80 fixed-in-net9.0-nightly This may be available in a nightly release! legacy-area-perf Startup / Runtime performance perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants