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

TodosApi updates #1836

Merged
merged 18 commits into from
May 2, 2023
Merged

TodosApi updates #1836

merged 18 commits into from
May 2, 2023

Conversation

DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Apr 21, 2023

  • Adds configuration binding in preparation for configuration binding source generator
  • Configured project so that OpenAPI document can be produced via a build target but OpenAPI code is removed at publish time via a runtime feature switch (this could potentially be integrated into Microsoft.AspNetCore.OpenApi)
    • e.g. in PowerShell: $env:ASPNETCORE_ENVIRONMENT='Build' ; dotnet build -p:OpenApiGenerateDocumentsOnBuild=true ; $env:ASPNETCORE_ENVIRONMENT=$null
  • Added launchSettings.json files to all the projects

Seems to have no performance impact:

load todosapi_aot todosapi_updates
CPU Usage (%) 12 13 +8.33%
Cores usage (%) 349 368 +5.44%
Working Set (MB) 48 48 0.00%
Private Memory (MB) 358 358 0.00%
Start Time (ms) 0 0
First Request (ms) 75 89 +18.67%
Requests/sec 72,701 74,347 +2.26%
Requests 1,096,144 1,121,051 +2.27%
Mean latency (ms) 4.22 4.09 -3.08%
Max latency (ms) 33.41 37.74 +12.96%
Bad responses 0 0
Socket errors 0 0
Read throughput (MB/s) 34.81 35.59 +2.24%
Latency 50th (ms) 1.79 1.83 +2.23%
Latency 75th (ms) 7.11 6.80 -4.36%
Latency 90th (ms) 10.29 9.85 -4.28%
Latency 99th (ms) 12.89 12.68 -1.63%

@@ -9,12 +9,27 @@
<LangVersion>preview</LangVersion>
<UserSecretsId>b8ffb8d3-b768-460b-ac1f-ef267c954c85</UserSecretsId>
<PublishAot>true</PublishAot>
<EnableLogging Condition="$(Configuration.StartsWith('Debug'))">true</EnableLogging>
<EnableConfigurationBindingGenerator>false</EnableConfigurationBindingGenerator>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we disabling this?

Copy link
Member Author

Choose a reason for hiding this comment

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

'Cos it's currently broken (it literally generates code with a compilation bug)

src/BenchmarksApps/TodosApi/TodosApi.csproj Outdated Show resolved Hide resolved
src/BenchmarksApps/TodosApi/Program.cs Outdated Show resolved Hide resolved
@DamianEdwards DamianEdwards force-pushed the damianedwards/stage2-configbinding branch from 282d82f to 5eb620c Compare April 25, 2023 21:30
@@ -8,7 +8,7 @@
"/throw": {
"get": {
"tags": [
"TodosApi"
"TodosApi, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"
Copy link
Member

Choose a reason for hiding this comment

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

This seems not ideal

Copy link
Member Author

@DamianEdwards DamianEdwards May 1, 2023

Choose a reason for hiding this comment

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

Oh interesting. I'm assuming you mean the tag seemingly being the full assembly name? This was produced by our own package (MS.Ext.ApiDescription.Server) but not sure why it changed. It's possible the previous version was produced by the Swashbuckle CLI and there's a subtle difference somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, is this from an old commit? The latest doesn't have this, perhaps it was the Swashbuckle CLI that added it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@DamianEdwards DamianEdwards force-pushed the damianedwards/stage2-configbinding branch from c8b51ca to 2f85f8b Compare May 1, 2023 16:13

internal static class HostEnvironmentExtensions
{
public static bool IsBuild(this IHostEnvironment hostEnvironment) => hostEnvironment.IsEnvironment("Build");
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this "Build" environment get defined/set?

Copy link
Member Author

@DamianEdwards DamianEdwards May 1, 2023

Choose a reason for hiding this comment

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

You have to set it manually via the ASPNETCORE_ENVIRONMENT env var. So e.g. in PowerShell to generate the swagger file you'd do this:

> $env:ASPNETCORE_ENVIRONMENT='Build' ; dotnet build -p:OpenApiGenerateDocumentsOnBuild=true ; $env:ASPNETCORE_ENVIRONMENT=$null

One could of course create a bash/PS/batch script to make this a simple invocation.

src/BenchmarksApps/TodosApi/appsettings.json Show resolved Hide resolved
src/BenchmarksApps/TodosApi/TodosApi.http Outdated Show resolved Hide resolved
<ItemGroup>
<Content Update="appSettings.Development.json" CopyToPublishDirectory="false" />
<!-- Workaround for https://github.com/dotnet/aspnetcore/issues/47941 -->
<IlcArg Include="--nopreinitstatics" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete this now that dotnet/runtime#85506 is fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Has that fixed flowed to installers yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe so, but we don't need it to. The benchmarks use the latest daily build of runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

It has not (talking about main here). While this would run on the perf infrastructure 'cos it pulls the latest individual runtime installers, it would be very difficult to run this on dev machines until the fix flows to the main SDK installer, so I think we shouldn't be removing workarounds until that happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's coming in dotnet/installer#16275. Hasn't been merged yet.

src/BenchmarksApps/TodosApi/DatabaseConfiguration.cs Outdated Show resolved Hide resolved
@DamianEdwards DamianEdwards merged commit 501d512 into main May 2, 2023
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.

3 participants