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

Show toolchain column when multiple toolchains are used #2121

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

YegorStepanov
Copy link
Contributor

@YegorStepanov YegorStepanov commented Sep 24, 2022

The 3rd commit shows what's changed.

I removed Summary.IsMultipleRuntimes because it is actually redundant.

Now the Toolchain column will be showed only when using .WithToolchain() method.

Job.Default.WithRuntime(Net60); // toolchain not showing
Job.Default.WithRuntime(Net60).WithToolchain(Net60); // toolchain showing

I tried shortening the approval filenames, but they are still long

@YegorStepanov YegorStepanov marked this pull request as draft September 26, 2022 17:48
@YegorStepanov YegorStepanov marked this pull request as ready for review September 28, 2022 04:49
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.

I am afraid this is trickier that it looks like.

Currently when I run

cd samples\BenchmarkDotNet.Samples
dotnet run -c Release -f net6.0 --filter *IntroBasic.Sleep --join --runtimes net462 net6.0 --job dry

I get:

image

With your changes the toolchain column becomes visible again:

image

We want the toolchain column to be visible only when there are at least two benchmarks that use the same Runtime, but different Toolchain.

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Oct 21, 2022

It looks that we should never display the Id column:

  1. All BDN attributes (like SimpleJob) set JobId, so we can't check IdCharacterisctic.
  2. We should not show Id if there is only one job, because it's already displayed under [Host].
  3. When Jobs have a significant difference (e.g. different InvocationCount), then the InvocationCount column should be displayed

I think we should disallow setting JobId by user, because now it only shows up in the logs. The random Id is enough.


[Theory]
[InlineData("nativeaot6.0", "NativeAOT 6.0", "ILCompiler 6.0.0-*")]
[InlineData("nativeaot7.0", "NativeAOT 7.0", "Latest ILCompiler")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is Latest ILCompiler the desired toolchain name? Should it be ILCompiler 7.0.0-*?

@YegorStepanov
Copy link
Contributor Author

Ready for review.

The only problem I found is:

[FactDotNetCoreOnly("In the .Net Framework cmd job uses CsProjClassicNetToolchain while fluent and attribute jobs use RoslynToolchain by default")]
[MethodImpl(MethodImplOptions.NoInlining)]
public void MultipleInputColumnsDisplayTest()
{
var cmdConfig = ConfigParser.Parse(
"--join --runtimes net481 net6.0 nativeaot6.0".Split(), NullLogger.Instance).config;

I didn't dare to change a default toolchain.

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Nov 22, 2022

  1. From Don't display Job and Toolchain column when running benchmarks for multiple runtimes #1603:

"The Job column should remain visible when the user has set the Job id in explicit way."

Done

  1. Changed runtime names when called from cmd:
    net50 -> .NET 5.0
  2. Changed tfm name when called from cmd:
    net50 -> net5.0 (to the correct tfm)
  3. Simplified name:
    InProcessEmitToolchain -> InProcessEmit (like other toolchains)

@YegorStepanov
Copy link
Contributor Author

Some possible improvements:

1) The [Host] section displays runtime info for each job Id.

This means that the [Host] section will only show the first job if multiple jobs have the same ID:

DefaultConfig.Instance
    .AddJob(Job.Dry)
    .AddJob(Job.Dry.WithRuntime(...)); // not displayed in the [Host] section

We can display the runtime info for each job.

2) When Id is not set, a random Id is used.

In such cases we can set the Id to be the difference between the jobs.

This is similar to how id is set using attributes. In this case, we do not need implicitId.

DefaultConfig.Instance
    .AddJob(Job.Dry)                     // id=Dry
    .AddJob(Job.Dry.WithRuntime(Net6.0)) // id=Dry-NET6.0

DefaultConfig.Instance
    .AddJob(Job.Dry.WithRuntime(Net6.0).WithInvocationCount(10)) // id=Dry-InvocationCount=10
    .AddJob(Job.Dry.WithRuntime(Net6.0)) // id=Dry

Cons:
When a new job is added, it can change another job Id (currently, the random Id is persistent)

3) Drop a point from runtime names

.NET 6.0 -> NET 6.0
.NET Framework 4.8.1 -> NET Framework 4.8.1

Look at the Host section and the Runtime column here

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