Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix SpanBench#9783

Closed
AndyAyersMS wants to merge 5 commits into
dotnet:masterfrom
AndyAyersMS:FixSpanBench
Closed

Fix SpanBench#9783
AndyAyersMS wants to merge 5 commits into
dotnet:masterfrom
AndyAyersMS:FixSpanBench

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

Make benchmark class public, add namespace. Tweak iteration counts
to very roughly 1 second per xunit-perf iteration.

Make benchmark class public, add namespace. Tweak iteration counts
to very roughly 1 second per xunit-perf iteration.
@AndyAyersMS
Copy link
Copy Markdown
Member Author

@joperezr @ahsonkhan PTAL
cc @dotnet/jit-contrib

Resolves the ongoing mystery of why this benchmark hasn't been producing perf results: when I originally wrote it I did not make the benchmark class public.

Local testing using run-xunit-perf shows reasonable results now.

namespace Span
{

public class Tests
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you rename this from Tests to something like SpanBench (or more descriptive and unique). Makes it easier to search, especially in benchview.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure

@ahsonkhan
Copy link
Copy Markdown

Looks good to me :)

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Meant to ping @jorive....

@jorive
Copy link
Copy Markdown

jorive commented Feb 25, 2017

@AndyAyersMS Changes look good. Optionally, you could change the benchmarks inner iterations to look more like this:

[Benchmark(InnerIterationCount = 10000)]
public static void TestFillAll()
{
    foreach (BenchmarkIteration iter in Benchmark.Iterations)
    {
        using (iter.StartMeasurement())
        {
            for (int i = 0; i < Benchmark.InnerIterationCount; ++i)

As-is, the results will display on BenchView like this: Span \ SpanBench \ TestFillAllSpan
I would suggest something more like: Span \ Benchmarks \ TestFillAll (remove the repetition)
What do you think?

@AndyAyersMS
Copy link
Copy Markdown
Member Author

The final Span suffix is there to distinguish the span cases from the corresponding array cases -- that is there is both a TestFillAllSpan and a TestFillAllArray.

This test can be run as a perf test two ways -- under xunit perf and also as a standalone exe. I generally haven't generally used inner iteration count for the tests I write to maintain symmetry with the exe-driven portion of the testing.

Happy to change the class name to Benchmarks if you think it is an improvement.

@ahsonkhan
Copy link
Copy Markdown

ahsonkhan commented Feb 25, 2017

@AndyAyersMS, I think SpanBench is fine.

There is already a method:

    static double Bench(Action f)

error CS0542: 'Bench': member names cannot be the same as their enclosing type [D:\j\workspace\debug_windows---17180f1b\tests\src\JIT\Performance\CodeQuality\Span\SpanBench.csproj]

@ahsonkhan
Copy link
Copy Markdown

@AndyAyersMS, you probably don't want to name your class "Benchmark"

SpanBench.cs(203,45): error CS0117: 'Benchmark' does not contain a definition for 'Iterations' [D:\j\workspace\checked_windo---f6dc6fe4\tests\src\JIT\Performance\CodeQuality\Span\SpanBench.csproj]

It gets confused with Benchmark attribute for the performance tests.

foreach (var iteration in Benchmark.Iterations)

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Odd that I don't see any of these errors building locally... anyways, back to SpanBench it is.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Arm emulator build failure, seems widespread since roughly midnight.

07:23:12 Copied 205 artifacts from "dotnet_coreclr » master » release_windows_nt_bld" build number 1215
07:23:12 ERROR: Unable to find project for artifact copy: dotnet_corefx/master/linuxarmemulator_softfp_cross_release
07:23:12 This may be due to incorrect project name or permission settings; see help for project name in job configuration

looks like this artifact comes from a corefx build, last good run has:

Copied 1 artifact from "dotnet_corefx » master » linuxarmemulator_softfp_cross_debug" build number 116

Root cause is likely this change which broke arm builds in CoreFX:
dotnet/corefx#16378

@AndyAyersMS
Copy link
Copy Markdown
Member Author

@ahsonkhan I'll abandon this since your #9790 has incorporated these changes already.

@hqueue
Copy link
Copy Markdown
Member

hqueue commented Feb 25, 2017

@dotnet-bot test Linux ARM Emulator Cross Release Build
@dotnet-bot test Linux ARM Emulator Cross Debug Build

@ahsonkhan
Copy link
Copy Markdown

I'll abandon this since your #9790 has incorporated these changes already.

Sounds good. Close this PR?

@AndyAyersMS AndyAyersMS closed this Mar 2, 2017
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants