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

Reduce allocations in Microsoft.Extensions.Configuration #88211

Merged
merged 3 commits into from
Jun 30, 2023

Conversation

pentp
Copy link
Contributor

@pentp pentp commented Jun 29, 2023

Currently calculating paths always allocates a string[] just to concatenate 3 strings.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 29, 2023
@ghost
Copy link

ghost commented Jun 29, 2023

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Currently calculating paths always allocates a string[] just to concatenate 3 strings.

Author: pentp
Assignees: -
Labels:

area-Extensions-Configuration, community-contribution

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Jun 29, 2023

@pentp did you run into any allocation problem to provide such fixes? or you just proactively looking for places we can optimize? Is it possible to provide some allocation numbers to see if it is worth optimizing? We are trying to be careful touch the configuration binding code as we experienced some regressions before.

@pentp
Copy link
Contributor Author

pentp commented Jun 29, 2023

@pentp did you run into any allocation problem to provide such fixes? or you just proactively looking for places we can optimize?

I just noticed that the configuration key (path) generation code was really wasteful when I was adopting a new configuration loading mechanisms for our app, but didn't gather any numbers.

@@ -69,11 +69,11 @@ public ConfigurationSection(IConfigurationRoot root, string path)
{
get
{
return _root[ConfigurationPath.Combine(Path, key)];
return _root[Path + ConfigurationPath.KeyDelimiter + key];
Copy link
Member

Choose a reason for hiding this comment

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

Here are three places where the lookup-key is created. Although it's trivial code, should this be moved to a helper method in order to have only one place where that lookup-key is built?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, should add an overload to ConfigurationPath.Combine that takes two path strings (the most common case), but adding it as an internal API wouldn't work because it's in a different assembly and adding a public API needs the full API review process. I'm willing to create an API review request for this, if it sounds useful enough?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think a public API is needed here. I thought just a helper in ConfigurationSection (as the PR's diff shows three places where that could be used).

pentp and others added 2 commits June 30, 2023 13:38
…src/ConfigurationPath.cs

Co-authored-by: Günther Foidl <gue@korporal.at>
…src/ConfigurationPath.cs

Co-authored-by: Günther Foidl <gue@korporal.at>
@tarekgh tarekgh added this to the 8.0.0 milestone Jun 30, 2023
@tarekgh tarekgh merged commit 7b13ea8 into dotnet:main Jun 30, 2023
103 checks passed
@pentp pentp deleted the config-path branch June 30, 2023 19:39
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration 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.

3 participants