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

Added PowerShell real-world benchmarks #3000

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented May 2, 2023

This is a snapshot of the PowerShell benchmarks found here.

We will update these as the PowerShell team updates their benchmarks.

cc @kunalspathak

@kunalspathak
Copy link
Member

We will update these as the PowerShell team updates their benchmarks.

We want to take the startup benchmark for sure (sometime next week), but after that probably we should update it once in a release so our test history maintains stable.

@kunalspathak
Copy link
Member

Can you add a small README.md?

@kunalspathak
Copy link
Member

@cincuranet
Copy link
Contributor

@kunalspathak Do you want to setup runs, auto-filing, etc.?

@kunalspathak
Copy link
Member

@kunalspathak Do you want to setup runs, auto-filing, etc.?

yes. We might want to add "startup" benchmark to the real world dashboard (when we add it).

kunalspathak
kunalspathak previously approved these changes May 3, 2023
Copy link
Member

@LoopedBard3 LoopedBard3 left a comment

Choose a reason for hiding this comment

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

Generally looks good, just a few questions to answer before merging. Also, there are no runs specifically setup currently. We can add the runs in a separate PR or in this PR, an example for adding the runs are https://github.com/dotnet/performance/blob/main/azure-pipelines.yml#L225-L238 for public runs, and https://github.com/dotnet/performance/blob/main/azure-pipelines.yml#L452-L467 for internal runs. Let me know and I can also help out for getting the runs setup 👍.

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#if NET6_0
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to benchmark these benchmarks on newer dotnet versions than Net6.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, yes we probably want to run these on versions newer than net6.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LoopedBard3 given these benchmarks are going to be copied verbatim from https://github.com/powershell/powershell - I think it's fine we can leave these benchmarks to be NET6.0 for now. The PowerShell team can update these benchmarks to allow newer versions.

Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure that I understand. In our lab, we always run against current which is .net8. Leaving it as .NET 6 will not execute this benchmark?

Copy link
Member

@LoopedBard3 LoopedBard3 May 8, 2023

Choose a reason for hiding this comment

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

That is correct as a whole. Benchmarks in the main branch are only run against whatever latest is. Each new release we make a branch to correspond to the previous release and hold the tests to be run against that release while it is in support, but we don't generally backport new tests to these branches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't. If the project is compiled to a target other than NET6, then these benchmarks will not be included.

@LoopedBard3
Copy link
Member

@cincuranet @caaavik-msft any reason not to add these as a new runkind (powershell)?

@cincuranet
Copy link
Contributor

cincuranet commented May 10, 2023

This has my full support.

@LoopedBard3
Copy link
Member

It looks like the ubuntu run is hitting the following error:

Assembly PowerShell.Benchmarks which defines benchmarks references non-optimized System.Management.Automation
If you own this dependency, please, build it in RELEASE.
If you don't, you can disable this policy by using 'config.WithOptions(ConfigOptions.DisableOptimizationsValidator)'.

Are you aware of whether the dependency is being built in RELEASE? Otherwise, we can disable the policy as long as we keep the understanding of the potential affect on the benchmarks.

@kunalspathak
Copy link
Member

@adityapatwardhan might know, but did you find out why it just fails on Ubuntu and not windows?

@LoopedBard3
Copy link
Member

@kunalspathak I have not found a reason why it only fails on Ubuntu and not Windows, and it is not something that I have any ideas about what could be causing it.

@adityapatwardhan
Copy link

looking

@adityapatwardhan
Copy link

The SDK package Microsoft.PowerShell.SDK has all assemblies as MSIL, is this the issue?

@adityapatwardhan
Copy link

adityapatwardhan commented May 18, 2023

@kunalspathak @LoopedBard3

Actually this could be the reason:

 <!-- Define non-windows, release configuration properties -->
  <PropertyGroup Condition=" '$(Configuration)' == 'Release' And '$(IsWindows)' != 'true' ">
    <!-- Set-Date fails with optimize enabled in NonWindowsSetDate
         Debugging the issues resolves the problem
     -->
    <Optimize>false</Optimize>
  </PropertyGroup>

https://github.com/PowerShell/PowerShell/blob/c444645b0941d73dc769f0bba6ab70d317bd51a9/PowerShell.Common.props#LL188C2-L194C19

@kunalspathak
Copy link
Member

Actually this could be the reason:

Thanks @adityapatwardhan . Not sure if I understand "Debugging the issues resolves the problem". Do you know what errors you get if we Optimize=true? Also, does that mean that powershell is not optimized at all for non-windows?

@adityapatwardhan
Copy link

This might be something very old. I am going to try and repro it.

@kunalspathak
Copy link
Member

This might be something very old. I am going to try and repro it.

Opened PowerShell/PowerShell#19677 to track it.

@TIHan
Copy link
Contributor Author

TIHan commented May 22, 2023

I've recently come back from vacation today, is there anything I can do to help with the failures on Ubuntu?

@kunalspathak
Copy link
Member

FYI- PowerShell/PowerShell#19701

@LoopedBard3
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Member

FYI- PowerShell/PowerShell#19701

This is merged. What are the next steps remaining here?

@LoopedBard3
Copy link
Member

FYI- PowerShell/PowerShell#19701

This is merged. What are the next steps remaining here?

It looks like the ubuntu run is still failing due to the Release vs Debug issue based on my recent run. The Microsoft.PowerShell.SDK version probably needs to be updated, although it does not look like there has been a recent update pushed out: https://www.nuget.org/packages/Microsoft.PowerShell.SDK/. @adityapatwardhan, do you have an idea of when the next package version with the update will be released or if there is a way to get a version with the changes?

Aside from that, it seems windows defender is stopping it on the public windows, but there was a working windows run before so it should not act as a blocker to merging. Although we may need to remove the public windows run until the issue is fixed.

@LoopedBard3
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LoopedBard3
Copy link
Member

@kunalspathak @TIHan It appears that the packages have been updated to the point that the debug errors are fixed and we should be able to add these benchmarks. I added a suggestion with the necessary package updates based on some of my testing: https://dev.azure.com/dnceng/internal/_build/results?buildId=2284858&view=results. With the package suggestion in and the pipeline run, we should be good to add these benchmarks.

@kunalspathak
Copy link
Member

Thanks @LoopedBard3 . If the CI is happy, please commit the suggested suggestions and merge the PR.

kunalspathak
kunalspathak previously approved these changes Oct 9, 2023
@kunalspathak
Copy link
Member

i am assuming the CI failures are unrelated. @TIHan we should add this benchmark in the superpmi collection.
CC: @BruceForstall

@TIHan
Copy link
Contributor Author

TIHan commented Oct 9, 2023

Yea that makes sense to me.

@LoopedBard3
Copy link
Member

i am assuming the CI failures are unrelated. @TIHan we should add this benchmark in the superpmi collection. CC: @BruceForstall

Is that in addition to here or instead of here?

@kunalspathak
Copy link
Member

Is that in addition to here or instead of here?

It is unrelated to this.

@LoopedBard3
Copy link
Member

Is that in addition to here or instead of here?

It is unrelated to this.

Cool, I will go ahead and merge this in then.

@LoopedBard3 LoopedBard3 merged commit a73d7fd into dotnet:main Oct 10, 2023
26 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants