Skip to content

Cache IResult types for well known status codes and results without parameters #39951

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

Closed
1 task done
davidfowl opened this issue Feb 3, 2022 · 7 comments · Fixed by #40965
Closed
1 task done

Cache IResult types for well known status codes and results without parameters #39951

davidfowl opened this issue Feb 3, 2022 · 7 comments · Fixed by #40965
Assignees
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Perf Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@davidfowl
Copy link
Member

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

We should optimize result allocation by caching common result instances (known status codes etc).

Describe the solution you'd like

Caching of these result types.

Additional context

No response

@davidfowl davidfowl added Perf old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Feb 3, 2022
@rafikiassumani-msft rafikiassumani-msft added this to the 7.0-preview4 milestone Feb 10, 2022
@rafikiassumani-msft rafikiassumani-msft added the Priority:1 Work that is critical for the release, but we could probably ship without label Feb 10, 2022
@brunolins16
Copy link
Member

brunolins16 commented Mar 24, 2022

I have started look this issue and those are the status/results that I am planning to have a PR to cache:

Always cache:

  • NoContentHttpResult : HTTP 204
  • UnauthorizedHttpResult : HTTP 401
  • EmptyResult: Already cached

Cache when no content (No parameters)

  • OkObjectHttpResult : HTTP 200
  • BadRequestObjectHttpResult : HTTP 400
  • NotFoundObjectHttpResult : HTTP 404
  • ConflictObjectHttpResult: HTTP 409
  • UnprocessableEntityObjectHttpResult: HTTP 422

@davidfowl
Copy link
Member Author

Also any know status code.

@brunolins16
Copy link
Member

Also any know status code.

@davidfowl is this list good enough:

`Status200OK`
`Status204NoContent`
`Status400BadRequest`
`Status401Unauthorized`
`Status403Forbidden`
`Status404NotFound`
`Status409Conflict`
`Status412PreconditionFailed` 
`Status415UnsupportedMediaType`
`Status500InternalServerError`

Or, you want a cache for every known status code listed here (maybe it is too much 😂):
https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http.Abstractions/src/StatusCodes.cs

I am planning to have an StatusCodeHttpResult cached instance for each of them, in addition to, what I mentioned before.

@davidfowl
Copy link
Member Author

Everything in that class.

@halter73
Copy link
Member

halter73 commented Mar 28, 2022

@davidfowl @brunolins16 Do you think we should codegen this? I'm not sure how much we do that kind of thing outside of Kestrel.

@brunolins16
Copy link
Member

@halter73 I am benchmarking different ideas for the caching, but I have a feeling that it might be a good idea to codegen whatever idea we end up with.

@davidfowl
Copy link
Member Author

Yes please t4?

@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Perf Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants