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 Vector<T> conversion benchmarks #1267

Merged
merged 2 commits into from
Apr 8, 2020

Conversation

saucecontrol
Copy link
Member

Adding these in preparation for work on dotnet/runtime#9766

Contributes to #1222

Copy link
Member

@adamsitnik adamsitnik left a 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.

@adamsitnik adamsitnik requested a review from tannergooding April 6, 2020 08:31
@adamsitnik
Copy link
Member

@tannergooding could you please take a look?

@@ -84,19 +84,26 @@ private static T GenerateValue<T>(Random random)
{
Copy link
Member

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)?

Copy link
Member Author

@saucecontrol saucecontrol Apr 6, 2020

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.

Copy link
Member

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
Copy link
Member

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)?

Copy link
Member

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.

Copy link
Member

@tannergooding tannergooding left a 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.

@adamsitnik adamsitnik merged commit 69bebad into dotnet:master Apr 8, 2020
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.

3 participants