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

Add a few more benchmarks #1449

Open
wants to merge 1 commit into
base: release/v6
Choose a base branch
from

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Dec 11, 2024

In order to better compare the performance between the existing v6 and the one with the Fractions I've been using these benchmarks. They are not very polished (and I haven't had the time for the string-related stuff) but they do the trick.

You don't have to merge them if you don't want to, but at least they are here (so I can reference the code).

I'll probably add the results from my local runs in a comment (or more like a bunch of comments) later on (my previous results got wiped by the build script 😆) - but I was hoping that we could update the packages and add net9.0 to the list of targets first (there are lots of a cool stuff in net9).

PS I'm not sure if that old gh-action would still work or not, but even adding the results by hand (here?) is an option.

Here's what I did for the Fractions (the Readme.md was 90% AI generated).

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Wow! A lot of great work here. It's more or less ready to merge, just one comment.

public TemperatureDelta SumOfDeltas()
{
#if NET
return UnitsNet.GenericMath.GenericMathExtensions.Sum(_quantities);
Copy link
Owner

Choose a reason for hiding this comment

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

I think using GenericMathExtension for net50+ and regular addition for net48 is an apples to oranges comparison, ref the #if NET condition.

I see that all the "Sum" tests involve GenericMathExtensions, but I think we should have dedicated tests for that class vs performance tests of the addition operators alone, to get meaningful results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my "Fraction" based implementation I've got the same extension prototype applicable to both types- so I'm comparing NET Framework v6 to NET Framework v6 with the new extension.
By itself the test only shows how much faster (ratio) the NET is compared to NET Framework when adding a bunch of quantities (also- the scaling doesn't linear with the fractions).

@angularsen
Copy link
Owner

Regarding the github action, the results back then were too variable to be useful. The benchmarks are fine, but we can't rely on github agents to do any consistent testing I think, unless they have come up with any special agents dedicated for performance testingd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants