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

Remove inner iteration counts from the benchmarks #126

Merged
merged 22 commits into from
Aug 29, 2018

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Aug 28, 2018

When I was porting the benchmarks I decided to keep the InnerIterationCount to prevent from chart scaling issue.

However now when @jorive wrote BenchView importer I realized that it does not make any sense to keep the InnerIterationCount.

Why is that? Let's consider following benchmark:

xunit style:

[Benchmark(InnerIterationCount = 7)]
public void Sleep()
{
    Benchmark.Iterate(() => Thread.Sleep(Timespan.FromMilliseconds(10)));
}

bdn style:

[Benchmark]
public void Sleep()
{
    for (int i = 0; i < 7; i++)
        Thread.Sleep(Timespan.FromMilliseconds(10));
}

When the data from xunit-performance gets reported to BenchView it's: "70 Duration (ms)" - so the metric is duration of entire iteration. InnerIterationCount does not matter, the result is not scaled.

BenchmarkDotNet scales the result and reports following results:

  1. 70 Duration of single invocation (ms) // to be honest the result is reported in nanoseconds
  2. 250 Duration (ms) - the duration of single iteration

Why the duration of single iteration is different for both tools? Because BenchmarkDotNet scales the number of operations per iteration according to the IterationTime setting, which for our repo is currently 250ms.

Summary: If I keep InnerIterationCount in the code the duration of single iteration will still be different. So I can remove it and don't worry about the scaling because we introduce new metric, so the historical data won't be affected.

adamsitnik and others added 20 commits August 27, 2018 18:44
…esult to make it consumable, simplify the setup
…ong benchmark where creating the tasks has huge overhead
[Benchmark]
public float LengthSquaredJitOptimizeCanaryBenchmark()
Copy link
Member Author

Choose a reason for hiding this comment

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

@jorive I have removed the *Canary* benchmarks because their purpose was to avoid the following pattern:

var result = VectorTests.Vector2Value;
for (var iteration = 0; iteration < VectorTests.DefaultInnerIterationsCount; iteration++)
{
    result = Vector2.Multiply(result, VectorTests.Vector2Delta);
}
return result;

To be optimized to:

return Vector2.Multiply(VectorTests.Vector2Value, VectorTests.Vector2Delta);

With BDN we have this out of the box, because it has it's own loop one level above and prevents from such elimination by preventing benchmark method inlining by using delegates.

Moreover, if one day JIT will become so smart that it will be able that Vector2.Multiply(VectorTests.Vector2Value, VectorTests.Vector2Delta) is a constant, BDN will report 0 in the results and it will be obvious to us what have happened

XmlDocument doc = _doc;

for (int i = 0; i < innerIterations; i++)
doc.LoadXml("<elem1 child1='' child2='duu' child3='e1;e2;' child4='a1' child5='goody'> text node two e1; text node three </elem1>");
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a bug: this benchmark was reusing the same instance of XmlDoc for all iterations and growing it with provided string in every iteration. So the time was raising with every benchmark invocation.

@adamsitnik adamsitnik requested a review from jorive August 28, 2018 12:30
@adamsitnik
Copy link
Member Author

@valenis I am removing the InnerIterationCount here (see the description above for full explanation)

@jorive
Copy link
Member

jorive commented Aug 28, 2018

@adamsitnik With these removal, do we know approx. how long does it take to run all benchmarks (CoreClr, CoreFx)? What's the difference before/after? Will these changes make the benchmarks "nano-benchmarks"?


[GlobalSetup(Target = nameof(ObjectGetTypeNoBoxing))]
public void SetupObjectGetTypeNoBoxing() => blackObject = Color.Black;
public void EnumCompareTo(Color color) => color.CompareTo(Color.White);
Copy link
Member

Choose a reason for hiding this comment

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

void [](start = 15, length = 4)

Shouldn't the return type be int?

Console.BackgroundColor = ConsoleColor.DarkGray;
Console.BackgroundColor = ConsoleColor.Red;
Console.BackgroundColor = ConsoleColor.DarkGreen;
Console.BackgroundColor = ConsoleColor.White;
}
Copy link
Member

@jorive jorive Aug 28, 2018

Choose a reason for hiding this comment

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

Aren't these two benchmarks testing assignment operation of an enum? Do we need them?
nvm... it's testing the PInvoke to the Pal layer. #Closed

@jorive
Copy link
Member

jorive commented Aug 28, 2018

// Licensed to the .NET Foundation under one or more agreements.

Given that we are renaming the benchmarks (creating new ones), wouldn't it be better to make this a generic?


Refers to: src/benchmarks/corefx/System.Numerics.Vectors/Perf_Vector2.cs:1 in 4000d50. [](commit_id = 4000d50, deletion_comment = False)

@jorive
Copy link
Member

jorive commented Aug 28, 2018

// Licensed to the .NET Foundation under one or more agreements.

Aren't we missing the compound operations? for example, *=, +=, -=


Refers to: src/benchmarks/corefx/System.Numerics.Vectors/Perf_Vector2.cs:1 in 4000d50. [](commit_id = 4000d50, deletion_comment = False)

Copy link
Member

@jorive jorive left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.

@adamsitnik
Copy link
Member Author

adamsitnik commented Aug 29, 2018

Given that we are renaming the benchmarks (creating new ones), wouldn't it be better to make this a generic?

For the vector operations it's not possible as of today:

  1. Vector is a struct so there is no base class and we can't do sth like
public class VectorTests<T> where T : VectorBase
{
    private T left = new T(), right = new T();

    [Benchmark] public T Multiply() => left.Multiply(right); // where Multiply would come from VectorBase
}
  1. There is no IVector<T> interface, so we can't do VectorTests<TI, T> where TI : IVector<T>
  2. C# does not allow to specify generic constraints based on available operators, maybe this will help one day Champion "Type Classes (aka Concepts, Structural Generic Constraints)" csharplang#110

@adamsitnik
Copy link
Member Author

Aren't we missing the compound operations? for example, *=, +=, -=

I think that the goal here was to compare Xmethod vs Xoperator. The removed benchmarks were using x= operators becase they were modifying the local variable to prevent from JIT optimization removing the loop.

…ionCount

# Conflicts:
#	src/benchmarks/corefx/System.Text.Encoding/Perf.Encoding.cs
@adamsitnik adamsitnik merged commit 212ab0d into dotnet:master Aug 29, 2018
@adamsitnik adamsitnik deleted the removeInnerIterationCount branch October 17, 2018 15:04
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.

None yet

2 participants