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 to .NET 7 #7698

Merged
merged 7 commits into from
Nov 27, 2022
Merged

Update to .NET 7 #7698

merged 7 commits into from
Nov 27, 2022

Conversation

benaadams
Copy link
Contributor

@benaadams benaadams commented Nov 14, 2022

@sebastienros
Copy link
Contributor

@roji do you want to do the db changes in a separate PR?
@EgorBo this PR contains <TieredPGO> and also envs in the docker file. Which option should we use for best throughput?

@benaadams
Copy link
Contributor Author

@EgorBo this PR contains <TieredPGO> and also envs in the docker file. Which option should we use for best throughput?

Shouldn't really matter both the same; .csproj is the more current way of doing it, and didn't feel like taking it out of 20 docker files 😅

@EgorBo
Copy link

EgorBo commented Nov 14, 2022

@roji do you want to do the db changes in a separate PR? @EgorBo this PR contains <TieredPGO> and also envs in the docker file. Which option should we use for best throughput?

I'd say for the best performance we also need DOTNET_ReadyToRun=0 that can't be defined via csproj. TieredPGO alone doesn't improve these benchmarks a lot because it can't instrumented R2R'd code of BCL and aspnet. We've fixed that in .NET 8.0 in dotnet/runtime#70941

image

But don't disable DOTNET_ReadyToRun if "time to first request" metric is important 🙂

_headerServer + _crlf +
_headerContentTypeHtml + _crlf +
_headerContentLength;
private static ReadOnlySpan<byte> _fortunesPreamble =>
Copy link

Choose a reason for hiding this comment

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

Glad to see AsciiString is no more 🎉

Copy link
Contributor Author

@benaadams benaadams Nov 14, 2022

Choose a reason for hiding this comment

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

C# caught up 😉

@benaadams
Copy link
Contributor Author

I'd say for the best performance we also need DOTNET_ReadyToRun=0 that can't be defined via csproj. TieredPGO alone doesn't improve these benchmarks a lot because it can't instrumented R2R'd code of BCL and aspnet. We've fixed that in .NET 8.0 in dotnet/runtime#70941

Already there

@davidfowl
Copy link
Contributor

@benaadams you can also improve

var utf8 = Encoding.ASCII.GetBytes("\r\nDate: ").AsSpan();
to use utf8 string literals.

@roji
Copy link
Contributor

roji commented Nov 15, 2022

@roji do you want to do the db changes in a separate PR?

@sebastienros sure, there's a bit work left on my side in any case (e.g. aspnet/Benchmarks#1764). Let's get this merged and I'll submit a separate database PR later.

@benaadams
Copy link
Contributor Author

@benaadams you can also improve

👍

@MichalStrehovsky
Copy link
Contributor

MichalStrehovsky commented Nov 21, 2022

@benaadams if you can run:

$ curl -L https://github.com/TechEmpower/FrameworkBenchmarks/commit/b0a0c17aaa94ff2187018f59656f83e10cea4c11.patch | git am

It would add support for PublishAot. I have that one in a separate pull request (#7720) and the CI already ran on it. Or that pull request can wait until this one merges, whenever that is.

We can also delete frameworks/CSharp/aspnetcore-corert because it ran its course (and has actually been broken for a while because it uses .NET 7 with ASP.NET 6 and there was a breaking change in Quic APIs).

@benaadams
Copy link
Contributor Author

@benaadams if you can run:

$ curl -L https://github.com/TechEmpower/FrameworkBenchmarks/commit/b0a0c17aaa94ff2187018f59656f83e10cea4c11.patch | git am

It would add support for PublishAot. I have that one in a separate pull request (#7720) and the CI already ran on it. Or that pull request can wait until this one merges, whenever that is.

We can also delete frameworks/CSharp/aspnetcore-corert because it ran its course (and has actually been broken for a while because it uses .NET 7 with ASP.NET 6 and there was a breaking change in Quic APIs).

Done and removed the corert framework; as suggested, since adding more variants that are equivalent

Copy link
Contributor

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@benaadams
Copy link
Contributor Author

NativeAOT is not currently supported for ASP.NET Core apps and thus this should not be marked as "Realistic".

Applied

@davidfowl
Copy link
Contributor

davidfowl commented Nov 21, 2022

@benaadams Can we change GetRequestType to use a switch expression on the path?

Ugh, it doesn't work with utf8 strings it seems

@benaadams benaadams mentioned this pull request Nov 25, 2022
@NateBrady23
Copy link
Member

@benaadams Let me know when this is ready!

@benaadams
Copy link
Contributor Author

@benaadams Let me know when this is ready!

Is good to go now; will do follow up for other changes

@NateBrady23 NateBrady23 merged commit 607afc3 into TechEmpower:master Nov 27, 2022
@benaadams benaadams deleted the net-7 branch January 27, 2023 16:40
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.

9 participants