Skip to content

Improve Clamp() performance#20051

Closed
symbiogenesis wants to merge 6 commits into
dotnet:mainfrom
symbiogenesis:clamp-performance
Closed

Improve Clamp() performance#20051
symbiogenesis wants to merge 6 commits into
dotnet:mainfrom
symbiogenesis:clamp-performance

Conversation

@symbiogenesis

@symbiogenesis symbiogenesis commented Jan 22, 2024

Copy link
Copy Markdown
Contributor

The current Clamp() extension based on IComparable is about twice as slow for floats and doubles, and was being used in at least 35 places.

It is just an internal function that was not used on anything interesting that implements IComparable. It only was used on int, double, and float types. And one DateTime.

The most critical path for clamp is probably Color.cs 

There already was another NumericExtensions class in the Graphics assembly for dealing with those types. Replicating the use of AggressiveInlining on the IComparable implementation, to align with NumericExtensions, doesn't change much about the results.

There is also Math.Clamp(), but it only exists on .NET Standard 2.1 and above. It allocates less for ints, but NumericExtensions is slightly better otherwise.

There is no MathF.Clamp() for floats, unfortunately.

Method Mean Error StdDev Median Allocated
ComparableClampInt 28.66 ms 0.503 ms 0.516 ms 28.64 ms 57 B
NativeClampInt 32.23 ms 1.564 ms 4.612 ms 29.56 ms 63 B
MathClampInt 27.72 ms 0.253 ms 0.211 ms 27.75 ms 12 B
Method Mean Error StdDev Median Allocated
ComparableClampFloat 100.16 ms 1.978 ms 2.117 ms 100.74 ms 100 B
NativeClampFloat 49.73 ms 0.699 ms 0.654 ms 49.72 ms 36 B
MathClampFloat 50.00 ms 0.773 ms 0.685 ms 49.95 ms 40 B
Method Mean Error StdDev Median Allocated
ComparableClampDouble 97.51 ms 1.717 ms 1.341 ms 97.53 ms 80 B
NativeClampDouble 47.52 ms 0.498 ms 0.466 ms 47.60 ms 40 B
MathClampDouble 50.25 ms 0.817 ms 0.764 ms 50.35 ms 40 B

@symbiogenesis symbiogenesis requested a review from a team as a code owner January 22, 2024 05:25
@ghost ghost added the community ✨ Community Contribution label Jan 22, 2024
@ghost

ghost commented Jan 22, 2024

Copy link
Copy Markdown

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.

@symbiogenesis symbiogenesis force-pushed the clamp-performance branch 3 times, most recently from 1a89f5a to 23dfcd7 Compare January 22, 2024 06:31
@symbiogenesis symbiogenesis marked this pull request as draft January 22, 2024 06:33
@symbiogenesis symbiogenesis force-pushed the clamp-performance branch 2 times, most recently from 3719619 to e80e4ea Compare January 22, 2024 07:23
@symbiogenesis symbiogenesis marked this pull request as ready for review January 22, 2024 07:52
@symbiogenesis

symbiogenesis commented Jan 22, 2024

Copy link
Copy Markdown
Contributor Author

Fixing the build, and aligning the ambiguous namespace resolution issue with PR #19965, which should be cleaner

@akhanalcs

Copy link
Copy Markdown

Loving your performance improvement PRs! Thank you!🙏

@symbiogenesis symbiogenesis force-pushed the clamp-performance branch 2 times, most recently from 461307f to dec28a0 Compare January 22, 2024 19:14
@symbiogenesis symbiogenesis changed the title Clamp performance Improve Clamp() performance Jan 22, 2024
@symbiogenesis symbiogenesis force-pushed the clamp-performance branch 4 times, most recently from 6262854 to 682bdd2 Compare January 23, 2024 02:57
@samhouts samhouts added the legacy-area-perf Startup / Runtime performance label Jan 25, 2024
@symbiogenesis

Copy link
Copy Markdown
Contributor Author

Ok, so I just noticed that there's double.Clamp() and float.Clamp() but they don't seem to use the same aggressive optimization techniques of Math.Clamp().

But I added them, because their performance was as good as anything else.

Comment on lines -7 to +9
public static T Clamp<T>(this T value, T min, T max) where T : IComparable<T>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static DateTime Clamp(this DateTime value, DateTime min, DateTime max)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So one thing to note is that [MethodImpl(MethodImplOptions.AggressiveInlining)] increases app size as the AOT images will be larger (libaot-Microsoft.Maui.Graphics.dll.so).

I would maybe not add it here if you didn't see it specifically improve something.

Comment on lines +1 to +3
#if NETSTANDARD2_1_OR_GREATER
using System;
#endif

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't seem like the #if expressions here are needed.

{
#if NETCOREAPP3_0_OR_GREATER
return double.Clamp(self, min, max);
#else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reading the docs, I think double/float.Clamp() were introduced in .NET 7?

https://learn.microsoft.com/en-us/dotnet/api/system.double.clamp?view=net-7.0

Comment on lines +1 to +4
using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("iOSUnitTests")]
[assembly: InternalsVisibleTo("Microsoft.Maui")]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to add this file, or can it be removed?

@symbiogenesis symbiogenesis marked this pull request as draft April 12, 2024 23:05
@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
@symbiogenesis symbiogenesis deleted the clamp-performance branch June 24, 2024 16:03
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 26, 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 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.

7 participants