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

Improved Shell benchmark #6167

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

pictos
Copy link
Contributor

@pictos pictos commented Apr 17, 2022

Description of Change

The benchmark method name GoTo passes the idea that we want to measure the Shell Navigation, but we're measuring more than that.

At this PR I moved the Shell initialization to a global method, to make sure that the benchmark will measure just the GoToAsync method. Before this change, the benchmark was measuring the shell initialization + navigation.

Main branch

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.22000
Intel Core i5-10500H CPU 2.50GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=6.0.300-preview.22204.3
  [Host]     : .NET Core 6.0.4 (CoreCLR 6.0.422.16404, CoreFX 6.0.422.16404), X64 RyuJIT
  DefaultJob : .NET Core 6.0.4 (CoreCLR 6.0.422.16404, CoreFX 6.0.422.16404), X64 RyuJIT

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
GoTo 66.74 μs 0.569 μs 0.475 μs 8.9111 0.7324 - 54.64 KB

This branch

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.22000
Intel Core i5-10500H CPU 2.50GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=6.0.300-preview.22204.3
  [Host]     : .NET Core 6.0.4 (CoreCLR 6.0.422.16404, CoreFX 6.0.422.16404), X64 RyuJIT
  DefaultJob : .NET Core 6.0.4 (CoreCLR 6.0.422.16404, CoreFX 6.0.422.16404), X64 RyuJIT

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
GoTo 10.01 μs 0.128 μs 0.120 μs 1.6479 - - 10.1 KB

As you can see the values changed a lot, indicating that we are measuring more than the navigation.

Issues Fixed

There's no issue for this, I can create one if needed.

Comment on lines -13 to +15
var shell = new Shell();
shell = new Shell();
Copy link
Member

Choose a reason for hiding this comment

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

At the time I wrote this benchmark, I was actually trying to make new Shell() faster...

This change is fine, though. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want I can create another PR to benchmark it. I've been working on some changes on GoToAsync to improve perf. and refactoring this way will be good to have a better notion about the changes.

@jonathanpeppers
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jonathanpeppers jonathanpeppers merged commit 4386167 into dotnet:main Apr 19, 2022
@pictos pictos deleted the pj/fix-benchmark-for-shell branch April 19, 2022 02:37
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
legacy-area-perf Startup / Runtime performance t/housekeeping ♻︎ t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants