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

Update AspNetCore benchmarks #4405

Merged

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Apr 19, 2023

Changes

  • Add instrumentation options Params to have a benchmark method run for each of the possible combinations:

    • No instrumentation enabled
    • Instrumentation enabled only for Traces
    • Instrumentation enabled only for Metrics
    • Instrumentation enabled for both Traces and Metrics
  • Update the web application builder setup to use async/await

  • Update BenchmarkDotNet version to 0.13.5

Benchmark results:

Method EnableInstrumentation Mean Error StdDev Gen0 Allocated
GetRequestForAspNetCoreApp None 226.8 us 4.00 us 3.74 us - 2.45 KB
GetRequestForAspNetCoreApp Traces 235.2 us 4.44 us 4.15 us 0.4883 3.59 KB
GetRequestForAspNetCoreApp Metrics 229.1 us 4.44 us 4.36 us - 2.92 KB
GetRequestForAspNetCoreApp Traces, Metrics 230.6 us 4.54 us 5.23 us 0.4883 3.66 KB

@utpilla utpilla requested a review from a team April 19, 2023 02:31
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #4405 (c174e1b) into main (457a9e9) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4405      +/-   ##
==========================================
- Coverage   84.75%   84.74%   -0.01%     
==========================================
  Files         300      300              
  Lines       12010    12010              
==========================================
- Hits        10179    10178       -1     
- Misses       1831     1832       +1     

see 3 files with indirect coverage changes

@cijothomas
Copy link
Member

@utpilla is the numbers steady or fluctuating a lot? its interesting that traces+metrics takes less time than traces alone. Is it always the case, or varies?

| GetRequestForAspNetCoreApp | None | 226.8 us | 4.00 us | 3.74 us | - | 2.45 KB |
| GetRequestForAspNetCoreApp | Traces | 235.2 us | 4.44 us | 4.15 us | 0.4883 | 3.59 KB |
| GetRequestForAspNetCoreApp | Metrics | 229.1 us | 4.44 us | 4.36 us | - | 2.92 KB |
| GetRequestForAspNetCoreApp | Traces, Metrics | 230.6 us | 4.54 us | 5.23 us | 0.4883 | 3.66 KB |
Copy link
Member

Choose a reason for hiding this comment

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

wondering why your machine took 230 us vs 171 us in my very very old (4th gen vs 9th gen) CPU ❓

Copy link
Member

Choose a reason for hiding this comment

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

also, the overhead is now 5-8% as opposed to 10-12% before.

Copy link
Contributor Author

@utpilla utpilla Apr 19, 2023

Choose a reason for hiding this comment

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

wondering why your machine took 230 us vs 171 us in my very very old (4th gen vs 9th gen) CPU ❓

Your old machine has a higher clock speed than my new machine 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, the overhead is now 5-8% as opposed to 10-12% before.

I have only made two major changes:

  1. The only Benchmarks tooling related difference here is that I have removed the InProcess attribute from the benchmark class. InProcess does not create a separate process for each benchmark. AFAIK, InProcess leads to a slightly better performance.
  2. The other difference in this PR is in the web application builder setup which now uses async/await when configuring the route: app.MapGet("/", async context => await context.Response.WriteAsync($"Hello World!")); instead of app.MapGet("/", () => $"Hello World!");. This leads to slightly lower memory allocation across the board (uninstrumented or otherwise). I haven't investigated why but I would think it's probably causing lesser allocation for async state machines in this case.

These are the results on my machine when I run the existing benchmarks in the main repo:

BenchmarkDotNet=v0.13.3, OS=Windows 11 (10.0.23424.1000)
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK=7.0.203
[Host] : .NET 7.0.5 (7.0.523.17405), X64 RyuJIT AVX2

Job=InProcess Toolchain=InProcessEmitToolchain

Method Mean Error StdDev Gen0 Allocated
UninstrumentedAspNetCoreApp 207.3 us 3.64 us 3.40 us 0.2441 2.54 KB
InstrumentedAspNetCoreAppWithDefaultOptions 210.1 us 1.93 us 1.81 us 0.4883 3.74 KB

It doesn't show a high overhead on my machine even for the existing ones 😀

Copy link
Member

Choose a reason for hiding this comment

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

Do you see any notable difference if you target net6.0 instead of net7.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.

My apologies! I missed this comment. I'll take a look at this as well.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't show a high overhead on my machine even for the existing ones

Yes. It simply means the benchmarks are not very steady, so can't be the sole way to measure overhead. I did see consistent numbers (showing 10+ % overhead).

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM. When will we see the AOT benchmarks?

@utpilla
Copy link
Contributor Author

utpilla commented Apr 19, 2023

LGTM. When will we see the AOT benchmarks?

I'm not sure if Benchmarks would be needed for AOT. Benchmark dotnet does a lot of warm up iterations anyway for jitting etc. to happen before it actually starts the measurements.

We would probably need some other kind of test that would also measure the startup time of the application.

@utpilla utpilla merged commit 586aa7e into open-telemetry:main Apr 19, 2023
@utpilla utpilla deleted the utpilla/Update-AspNetCoreBenchmarks branch November 3, 2023 22:12
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.

4 participants