-
Notifications
You must be signed in to change notification settings - Fork 274
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 Vector<T> conversion benchmarks #1267
Conversation
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.
@saucecontrol thank you for benchmarks! They look good from benchmarking perspective, I have added some comments but mostly nit styling stuff.
src/benchmarks/micro/libraries/System.Numerics.Vectors/Perf_VectorConvert.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Numerics.Vectors/Perf_VectorConvert.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Numerics.Vectors/Perf_VectorConvert.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Numerics.Vectors/Perf_VectorConvert.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Numerics.Vectors/Perf_VectorConvert.cs
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Numerics.Vectors/Perf_VectorConvert.cs
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Numerics.Vectors/Perf_VectorConvert.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Numerics.Vectors/Perf_VectorConvert.cs
Outdated
Show resolved
Hide resolved
@tannergooding could you please take a look? |
@@ -84,19 +84,26 @@ private static T GenerateValue<T>(Random random) | |||
{ |
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 it be better to make some of these return their full range (at least the unsigned ones since Random.Next()
doesn't return negatives)?
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.
I was going for consistency with the existing uint
and ulong
ones, but I see they were just added last week. Probably wouldn't be bad to change those now. One other issue I noticed is that the double
(and now float
) ones return values between 0 and 1, which is great for most floating point tests, but when you're converting those to integer values, it doesn't give you much coverage.
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.
Yeah. I would think ideally we'd have some level of configuration that allows returning negative values
and values within the entire floating-point
domain.
The latter would likely still be random, but you wouldn't have "even distribution" (since floating-point
isn't evenly distributed).
[Benchmark] | ||
public Vector<ulong> Widen_uint() => Widen<uint, ulong>(s_valuesUInt); | ||
|
||
private static Vector<TTo> Convert<TFrom, TTo>(TFrom[] values) where TFrom : struct where TTo : struct |
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.
@adamsitnik, is there an issue on BDN that tracks adding better support for measuring operations that take less time than the timer frequency?
Likewise is there something that would more readily allow "random" inputs within some domain for those operations (for both primitive and structured data)?
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.
is there an issue on BDN that tracks adding better support for measuring operations that take less time than the timer frequency?
I think that we are doing quite OK with things that take less time than the timer frequency, the problem might be benchmarks that take less than 1ns. Do you have any concrete example of an operation that is hard to benchmark properly as of today? We could create an issue and start a discussion with @AndreyAkinshin
BTW for this benchmark OperationsPerInvoke could be handy.
Likewise is there something that would more readily allow "random" inputs within some domain for those operations (for both primitive and structured data)?
No. We rather expect the users to generate such data and put it into an array during a setup, and then just iterate over it during benchmark.
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.
LGTM.
I left a couple comments about the random value ranges and one on improved BDN for scenarios like this.
Adding these in preparation for work on dotnet/runtime#9766
Contributes to #1222