Caching common HttpResults types by brunolins16 · Pull Request #40965 · dotnet/aspnetcore

brunolins16 · 2022-03-30T23:08:14Z

In this PR is introduced a new partial class ResultsCache that contains the following common HttpResults cached instances:

NoContentHttpResult : HTTP 204
UnauthorizedHttpResult : HTTP 401
OkObjectHttpResult : HTTP 200 **
BadRequestObjectHttpResult : HTTP 400 **
NotFoundObjectHttpResult : HTTP 404 **
ConflictObjectHttpResult: HTTP 409 **
UnprocessableEntityObjectHttpResult: HTTP 422 **

** with null content

It also includes an auto generated (T4 text template) addition to this new partial class that will handle caching for StatusCodeHttpResult. This cache will use a simple switch expression with lazy object allocation based on the benchmarks listed below.

In general, the benchmarks shown that an improvement in the execution time when a known status code but a degradation when an unknown status code, however, the difference is around 1 ns or 2 ns that might not affect most of the scenarios. Also, as expected, the cache will avoid the continuous StatusCodeHttpResult object allocation (24 bytes) that happens every request.

Fix #39951

Micro-benchmark results

All types used for the benchmarks are listed here: https://gist.github.com/brunolins16/82593d2f24ae63d602baa902b09a5ed3
Also, all benchmarks listed here are related to the StatusCodeHttpResult caching only.

Initial allocation cost

Method	Gen 0	Gen 1	Allocated
NoCache	-	-	-
StaticCacheWithDictionary	0.0286	-	6,208 B
DynamicCacheWithFixedSizeArray	0.0200	0.0002	4,144 B
DynamicCacheWithFixedSizeArrayPerStatusGroup	0.0042	-	864 B
DynamicCacheWithConcurrentDictionary	0.0048	-	992 B
StaticCacheWithSwitchExpression	0.0095	-	2,000 B
DynamicCacheWithSwitchExpression	0.0001	-	24 B

Known status code (Eg. HTTP 200)

Method	Mean	Error	StdDev	Gen 0	Allocated
NoCache	2.725 ns	0.0285 ns	0.0253 ns	0.0001	24 B
StaticCacheWithDictionary	5.733 ns	0.0373 ns	0.0331 ns	-	-
DynamicCacheWithFixedSizeArray	2.184 ns	0.0227 ns	0.0212 ns	-	-
DynamicCacheWithFixedSizeArrayPerStatusGroup	3.371 ns	0.0151 ns	0.0134 ns	-	-
DynamicCacheWithConcurrentDictionary	5.450 ns	0.1495 ns	0.1468 ns	-	-
StaticCacheWithSwitchExpression	1.867 ns	0.0045 ns	0.0042 ns	-	-
DynamicCacheWithSwitchExpression	1.889 ns	0.0143 ns	0.0119 ns	-	-

Unknown status code (Eg. HTTP 150)

Method	Mean	Error	StdDev	Gen 0	Allocated
NoCache	2.477 ns	0.0818 ns	0.1005 ns	0.0001	24 B
StaticCacheWithDictionary	8.479 ns	0.0650 ns	0.0576 ns	0.0001	24 B
DynamicCacheWithFixedSizeArray	2.234 ns	0.0361 ns	0.0302 ns	-	-
DynamicCacheWithFixedSizeArrayPerStatusGroup	4.809 ns	0.0360 ns	0.0281 ns	0.0001	24 B
DynamicCacheWithConcurrentDictionary	6.076 ns	0.0672 ns	0.0595 ns	-	-
StaticCacheWithSwitchExpression	4.195 ns	0.0823 ns	0.0770 ns	0.0001	24 B
DynamicCacheWithSwitchExpression	4.146 ns	0.0401 ns	0.0335 ns	0.0001	24 B

Status code outside of the documented range (100..599) (Eg. HTTP 900)

Method	Mean	Error	StdDev	Gen 0	Allocated
NoCache	2.566 ns	0.0389 ns	0.0364 ns	0.0001	24 B
StaticCacheWithDictionary	6.997 ns	0.0647 ns	0.0574 ns	0.0001	24 B
DynamicCacheWithFixedSizeArray	3.886 ns	0.0170 ns	0.0159 ns	0.0001	24 B
DynamicCacheWithFixedSizeArrayPerStatusGroup	3.627 ns	0.0465 ns	0.0412 ns	0.0001	24 B
DynamicCacheWithConcurrentDictionary	5.541 ns	0.0661 ns	0.0619 ns	-	-
StaticCacheWithSwitchExpression	3.127 ns	0.0554 ns	0.0491 ns	0.0001	24 B
DynamicCacheWithSwitchExpression	3.258 ns	0.0261 ns	0.0244 ns	0.0001	24 B

davidfowl · 2022-03-31T00:30:30Z

+
+    internal static StatusCodeHttpResult StatusCode(int statusCode)
+    {
+        if ( statusCode is (< 100) or (> 511))


Suggested change

if ( statusCode is (< 100) or (> 511))

if (statusCode is (< 100) or (> 511))

For reference: Perf-goodness will come for free with dotnet/roslyn#60534

pranavkm · 2022-03-31T01:29:58Z

+
+    internal static StatusCodeHttpResult StatusCode(int statusCode)
+    {
+        if ( statusCode is (< 100) or (> 511))


Suggested change

if ( statusCode is (< 100) or (> 511))

if (statusCode is (< 100) or (> 511))

pranavkm · 2022-03-31T03:30:48Z

+
+internal static partial class ResultsCache
+{
+    public static NotFoundObjectHttpResult NotFound { get; } = new(null);


Out of curiosity, why not align this with the other status code results? You generally don't need to be concerned with back-compat of implementation particularly when all of it was previously non-public.

why not align this with the other status code results?

I did not follow, sorry, what do you mean about it?

@pranavkm As of right now, we've decided to avoid a type hierarchy. And I don't think we want NotFound(object? value = null) returning a StatusCodeHttpResult in some cases and a NotFoundObjectHttpResult in others.

pranavkm · 2022-03-31T03:33:08Z

@@ -0,0 +1,108 @@
+<#@ template debug="true" hostspecific="true" language="C#" #>


I imagine this limits you to having to use VS to regen the file. Have you considered doing something different (e.g. https://github.com/dotnet/aspnetcore/tree/main/src/Servers/Kestrel/tools/CodeGenerator) which works everywhere? Alternatively now that the generated code is available, use that to drive further work and not check this in?

If I am not wrong it is already supported in Rider and probably should have some VS Code extension available but I could check the alternative that you mentioned.

I think it's fine to use t4.

Also, a quick search let me find the Mono.TextTemplating project https://github.com/mono/t4. That is a community dotnet tool that allows the transformation of the T4 templates.

Yes, @mhutch owns this

gfoidl · 2022-03-31T09:02:14Z

+[GeneratedCode("TextTemplatingFileGenerator", "")]
+internal partial class ResultsCache
+{
+    private static StatusCodeHttpResult? _status100Continue;


Should these be static readonlys

Suggested change

private static StatusCodeHttpResult? _status100Continue;

private static readonly StatusCodeHttpResult? s_status100Continue = new(StatusCodes.Status100Continue);

And in the switch below

return statusCode switch { StatusCodes.Status100Continue => s_status100Continue, ...

That way the lazy-initialization is shifted to the runtime, and by beforefieldinit it's done on first access.

With tiered-compilation hot StatusCodeHttpResults could already be initialized, so no extra (runtime-) check is needed anymore.
Otherwise it's pretty the same if there's a ?? new(StatusCodes.XYZ) or if that check is done by the runtime (I guess).

Of course the change in the tt-file below.

@gfoidl I really appreciate your comment. I did not try the tiered-compilation thing yet however the beforefieldinit indeed do a lazy-initialization but based on my tests when the initialization happens all the ~ 60 fields are initialized and all memory is allocated. The performance improvement is minimum compared with the null check approach.

Initial allocation cost (First time access)

Method Gen 0 Gen 1 Allocated

StaticCacheWithSwitchExpression 0.0095 - 2,000 B

DynamicCacheWithSwitchExpression 0.0001 - 24 B

Method Mean Error StdDev Gen 0 Allocated

StaticCacheWithSwitchExpression 1.867 ns 0.0045 ns 0.0042 ns - -

DynamicCacheWithSwitchExpression 1.889 ns 0.0143 ns 0.0119 ns - -

Drop _status100Continue, that's a response code that's only ever returned by the server, not the app.

davidfowl · 2022-04-01T02:45:15Z

+
+  <ItemGroup>
+    <!-- This is the T4 template service and is added by VS anytime you modify a T4 template. Required for .tt files. -->
+    <Service Include="{508349b6-6b84-4df5-91f0-309beebad82d}" />


ew, but OK 😄

I don't like it too, and I can actually remove but it will come back, and the next dev will need to remove it again. If that is fine i would prefer to remove it, including the Compile/None items added by VS.

halter73

If it's easy enough, it'd be nice to add a test to make sure the source and T4 template don't go out of sync. And please add something to the src/Http/README.md how to rerun the T4 template using the dotnet-t4 tool for people without VS.

brunolins16 · 2022-04-07T04:15:09Z

If it's easy enough, it'd be nice to add a test to make sure the source and T4 template don't go out of sync. And please add something to the src/Http/README.md how to rerun the T4 template using the dotnet-t4 tool for people without VS.

@halter73 I added the unit tests and updated the readme. can you please take a quick look?

5982d23
9cc0107

Co-authored-by: Günther Foidl <gue@korporal.at>

BrennanConroy · 2022-04-07T15:53:46Z

+
+    internal static StatusCodeHttpResult StatusCode(int statusCode)
+    {
+        if (statusCode is (< 100) or (> 599))


Suggested change

if (statusCode is (< 100) or (> 599))

if (statusCode is (< 101) or (> 599))

Co-authored-by: Brennan <brecon@microsoft.com>

dougbu · 2022-04-07T05:21:36Z

+        => value is null ? ResultsCache.NotFound : new NotFoundObjectHttpResult(value);

    /// <summary>
    /// Produces a <see cref="StatusCodes.Status401Unauthorized"/> response.


Doesn't this actually return a cached UnauthorizedHttpResult instance❔ Same for other special cases in src/Http/Http.Results/src/ResultsCache.cs e.g. the NotFoundObjectHttpResult just above.

Sorry, I am confusing about your question. This will return a cached NotFoundObjectHttpResult not UnauthorizedHttpResult. Maybe I am missing something.

My comment is about the <summary/> after the overload that returns NotFoundObjectHttpResult. I probably should have commented on that earlier overload.

In any case, I was confused and thought the <summary/> was about the return type. StatusCodes.Status404NotFound, StatusCodes.Status401Unauthorized, et cetera are of course ints not IResults.

In retrospect, my suggestion should have been to make the return type (NotFoundObjectHttpResult, UnauthorizedHttpResult, et cetera) clear as well.

I agree about the return type but that is a problem that we have right now because this class was shipped already but we are working on it #41009

My suggestion was about the doc comments, not the method signature e.g.

Suggested change

/// Produces a <see cref="StatusCodes.Status401Unauthorized"/> response.

/// Returns an <see cref="UnauthorizedHttpResult"/> that produces a <see cref="StatusCodes.Status401Unauthorized"/> response.

Again, this is a suggestion and could be done later. You're not otherwise changing return types or updating <summary/> elements in this PR.

brunolins16 added 8 commits March 29, 2022 13:13

Caching known results

ef3e263

removing generated code

7045da0

Updating files

ef57ac4

Updating csproj

3379796

Updating autogenerated code

b7d7273

Removing non-lazy cache

2fe11a9

formatting fix

09439cb

small perf condition

99da26a

ghost added the area-runtime label Mar 30, 2022

small perf

beb2940

brunolins16 requested a review from a team March 30, 2022 23:57

brunolins16 marked this pull request as ready for review March 30, 2022 23:57

brunolins16 requested review from BrennanConroy, Tratcher and halter73 as code owners March 30, 2022 23:57

brunolins16 added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Mar 30, 2022

davidfowl reviewed Mar 31, 2022

View reviewed changes

pranavkm reviewed Mar 31, 2022

View reviewed changes

brunolins16 added 3 commits March 30, 2022 22:30

Fixing formatting

5b264bd

Range adjustment

4eaafae

removing template debug flag

0baebb2

gfoidl reviewed Mar 31, 2022

View reviewed changes

davidfowl reviewed Apr 1, 2022

View reviewed changes

Removing HTTP 100

c363cbc

halter73 approved these changes Apr 6, 2022

View reviewed changes

Adding unit test for t4 template

5982d23

brunolins16 requested review from a team, dougbu and wtgodbe as code owners April 7, 2022 00:54

Fixing ubuntu test

9cc0107

gfoidl reviewed Apr 7, 2022

View reviewed changes

Comment thread src/Http/README.md Outdated

Update src/Http/README.md

8e20ad1

Co-authored-by: Günther Foidl <gue@korporal.at>

BrennanConroy approved these changes Apr 7, 2022

View reviewed changes

Update src/Http/Http.Results/test/ResultsCacheTests.cs

1c44fad

Co-authored-by: Brennan <brecon@microsoft.com>

dougbu reviewed Apr 7, 2022

View reviewed changes

brunolins16 and others added 2 commits April 7, 2022 12:53

PR Feedback

8e23453

Merge branch 'main' into brunolins16/issues/39951

1f72319

brunolins16 enabled auto-merge (squash) April 8, 2022 00:11

brunolins16 merged commit e9b2adb into dotnet:main Apr 8, 2022

ghost added this to the 7.0-preview4 milestone Apr 8, 2022

brunolins16 deleted the brunolins16/issues/39951 branch August 2, 2022 20:49

davidfowl added the Perf label Aug 26, 2022

Rick-Anderson mentioned this pull request Oct 8, 2022

IResult implementation types are now publicly available dotnet/AspNetCore.Docs.Samples#92

Closed

	if ( statusCode is (< 100) or (> 511))
	if (statusCode is (< 100) or (> 511))

		@@ -0,0 +1,108 @@
		<#@ template debug="true" hostspecific="true" language="C#" #>

	private static StatusCodeHttpResult? _status100Continue;
	private static readonly StatusCodeHttpResult? s_status100Continue = new(StatusCodes.Status100Continue);

	if (statusCode is (< 100) or (> 599))
	if (statusCode is (< 101) or (> 599))

	/// Produces a <see cref="StatusCodes.Status401Unauthorized"/> response.
	/// Returns an <see cref="UnauthorizedHttpResult"/> that produces a <see cref="StatusCodes.Status401Unauthorized"/> response.

Conversation

brunolins16 commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Micro-benchmark results

Initial allocation cost

Known status code (Eg. HTTP 200)

Unknown status code (Eg. HTTP 150)

Status code outside of the documented range (100..599) (Eg. HTTP 900)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

brunolins16 commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunolins16 Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

brunolins16 commented Mar 30, 2022 •

edited

Loading

brunolins16 commented Apr 7, 2022 •

edited

Loading

brunolins16 Apr 7, 2022 •

edited

Loading