Optimize the performance of InternalConfigurationRootExtensions.GetChildrenImplementation #74736
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-extensions-configuration Issue Details@rjgotten I implemented your design at #65885 The benchmark of dotnet/performance#2572 is improved to
|
wangyuantao
changed the title
src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
wangyuantao
changed the title
src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs
Outdated
Show resolved
Hide resolved
Wow. Just. Wow |
The challenging part is how to make the back compat perfect. Actually I'm not very confident about current change, because the behavior of the GetChildren changed - previously the final keys order is determined by the last provider, now with my change, the keys order is determined by the us internally which is always sorted. If some clients hack the provider and use the last provider to output the keys in a specific order, now they cannot depend on that specific order... |
| @@ -24,11 +24,15 @@ internal static IEnumerable<IConfigurationSection> GetChildrenImplementation(thi | |||
| IEnumerable<IConfigurationProvider> providers = reference?.Providers ?? root.Providers; | |||
|
|
|||
| IEnumerable<IConfigurationSection> children = providers | |||
| #if NET7_0_OR_GREATER | |||
| .SelectMany(p => p.GetChildKeys(path)) | |||
There was a problem hiding this comment.
This is tantamount to the following unless we take advantage of the new GetChildKeys interface method in our ConfigurationProvider and ChainedConfigurationProvider classes to stop sorting since that would be non-breaking.
| .SelectMany(p => p.GetChildKeys(path)) | |
| .SelectMany(p => p.GetChildKeys(Enumerable.Empty<string>(), path)) |
If the problem is just the bad asymptotic cost of resorting previous keys for each additional provider, and not sorting keys internally, I don't think we need the new interface.
However, we then still need to sort all the keys at the end of the GetChildrenImplementation to avoid breaking callers that expect sorted keys from default IConfiguration.GetChildren() implementations.
The challenging part is how to make the back compat perfect. Actually I'm not very confident about current change, because the behavior of the GetChildren changed - previously the final keys order is determined by the last provider, now with my change, the keys order is determined by the us internally which is always sorted. If some clients hack the provider and use the last provider to output the keys in a specific order, now they cannot depend on that specific order...
I'm not sure we need to be worried about developers using the last provider to hijack the earlierKeys from earlier providers. Does anyone know of anything that does this?
There was a problem hiding this comment.
Another idea for back compat is to let caller decide the implementation. For commonly used caller like Binder, we know the optimized path can be used for sure. How do you think? Send a new iteration for brainstorming.
…onfigurationRootExtensions.cs Agree. No need to introduce new interface. Co-authored-by: Stephen Halter <[email protected]>
src/libraries/Microsoft.Extensions.Configuration/ref/Microsoft.Extensions.Configuration.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs
Outdated
Show resolved
Hide resolved
|
We are closing this since we haven't decided about the best way to handle this optimization. We can still discuss more as needed for how we can do it. |
ghost
locked as resolved and limited conversation to collaborators
@rjgotten I implemented your design at #65885
The benchmark of dotnet/performance#2572 shows (-c Release -f net7.0)
Before
After