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

Enumerate paramnames on startup #46086

Merged
merged 2 commits into from
Jan 23, 2023

Conversation

marafiq
Copy link
Contributor

@marafiq marafiq commented Jan 13, 2023

Enumerate endpoint parameter names on startup instead of allocating a list.

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes

Use Enumerable instead of List to save allocation

Use of IEnumerable<string> when getting the endpoint parameters names from the collection, which then gets used to create the RequestDelegateFactoryContext

Fixes #46010

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Jan 13, 2023
@ghost
Copy link

ghost commented Jan 13, 2023

Thanks for your PR, @marafiq. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dnfadmin
Copy link

dnfadmin commented Jan 13, 2023

CLA assistant check
All CLA requirements met.

@adityamandaleeka
Copy link
Member

@mitchdenny Assigning this to you to look at since the corresponding issue is assigned to you.

@mitchdenny
Copy link
Member

Also looping in @captainsafia since she would be interested in seeing the PR and is doing some stuff in AOT in this area.

On the surface this doesn't look like a breaking API change (good) - but in terms of performance impact in an actual application I don't think I can see the impact. One thing to point out with the benchmark is that you've got 10 route parameters - that is actually a lot when you consider even quite nested APIs such as:

/customers/{customer}/contacts/{contact}/addresses/{address}

I re-ran the benchmark with 3 route parameters and the results aren't quite as dramatic in that case:

|                          Method |      Mean |     Error |    StdDev |   Gen0 | Allocated |
|-------------------------------- |----------:|----------:|----------:|-------:|----------:|
|              Add10StringsToList | 58.127 ns | 1.1880 ns | 1.7414 ns | 0.0278 |     120 B |
|             Add10StringsToArray | 49.984 ns | 0.8905 ns | 0.8330 ns | 0.0204 |      88 B |
|     UseEnumerableWithLinqSelect | 21.989 ns | 0.3592 ns | 0.2999 ns | 0.0167 |      72 B |
| UseEnumerableWithLocalFuncYield |  9.093 ns | 0.2014 ns | 0.1978 ns | 0.0111 |      48 B |

Let's imagine an application with a modest number of 3 parameter routes (say a 10):

Scenario Startup time contribution (10 * mean ns) Memory contribution (10 * mean allocations)
Without optimization 580 nanoseconds 1200 bytes
With optimization 90 nanoseconds 480 bytes

@marafiq
Copy link
Contributor Author

marafiq commented Jan 17, 2023

Thanks for taking a look at us @mitchdenny
Three route parameters for an endpoint are a good default. One thing to note is that the code will run for every endpoint with parameters.

but in terms of performance impact in an actual application I don't think I can see the impact

As you have seen in your benchmark run, there are slight improvements in allocation and some ns. There is not nothing else in terms of impact.

@mitchdenny mitchdenny marked this pull request as ready for review January 23, 2023 20:49
Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

We will probably not merge this kind of micro-optimization unless it is on a really hot path in the future. If the optimization is large enough that it would show a statistically significant impact on the overall RequestDelegateFactory.Create(Delegate) call, that would be a different story.

That said, I appreciate the initiative! Given that the microbenchmark shows an improvement, and that the code is as easy to follow as it was before imo, I think we should merge the PR this time.

Thanks for contributing!

@mitchdenny mitchdenny merged commit 9a118ce into dotnet:main Jan 23, 2023
@ghost ghost added this to the 8.0-preview1 milestone Jan 23, 2023
atossell91 pushed a commit to atossell91/aspnetcore that referenced this pull request Jan 23, 2023
Enumerate endpoint parameter names on startup instead of allocating a list.
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array will allocate less and little faster when creating RequestDelegateFactoryOptions in CreateRDFOptions
6 participants