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

Optimize the performance of InternalConfigurationRootExtensions.GetChildrenImplementation #74736

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ public partial interface IConfigurationBuilder
public partial interface IConfigurationProvider
{
System.Collections.Generic.IEnumerable<string> GetChildKeys(System.Collections.Generic.IEnumerable<string> earlierKeys, string? parentPath);
#if NET7_0_OR_GREATER
System.Collections.Generic.IEnumerable<string> GetChildKeys(string? parentPath)
=> GetChildKeys(System.Linq.Enumerable.Empty<string>(), parentPath);
#endif
Microsoft.Extensions.Primitives.IChangeToken GetReloadToken();
void Load();
void Set(string key, string? value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,16 @@ public interface IConfigurationProvider
/// <param name="parentPath">The parent path.</param>
/// <returns>The child keys.</returns>
IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath);

#if NET7_0_OR_GREATER
/// <summary>
/// Returns the configuration keys for a given parent path based on this
/// <see cref="IConfigurationProvider"/> data.
/// </summary>
/// <param name="parentPath">The parent path.</param>
/// <returns>The child keys.</returns>
IEnumerable<string> GetChildKeys(string? parentPath)
=> GetChildKeys(System.Linq.Enumerable.Empty<string>(), parentPath);
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Suggested change
.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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

.OrderBy(key => key, ConfigurationKeyComparer.Instance)
#else
.Aggregate(Enumerable.Empty<string>(),
(seed, source) => source.GetChildKeys(seed, path))
#endif
.Distinct(StringComparer.OrdinalIgnoreCase)
.Select(key => root.GetSection(path == null ? key : ConfigurationPath.Combine(path, key)));

if (reference is null)
{
return children;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -971,5 +971,145 @@ public void ProviderWithNullReloadToken()
// Assert
Assert.NotNull(config);
}

[Fact]
public void ConfigurationGetChildrenKeysOrdered()
{
// Arrange
var dic1 = new Dictionary<string, string>()
{
{"Services:2", "Service2"}
};
var dic2 = new Dictionary<string, string>()
{
{"Services:10", "Service10"},
};
var memConfigSrc1 = new MemoryConfigurationSource { InitialData = dic1 };
var memConfigSrc2 = new MemoryConfigurationSource { InitialData = dic2 };

var configurationBuilder = new ConfigurationBuilder();
configurationBuilder.Add(memConfigSrc1);
configurationBuilder.Add(memConfigSrc2);

var config = configurationBuilder.Build();

// Act
var keys = config.GetSection("Services").GetChildren().Select(c => c.Key).ToList();

// Assert
Assert.Equal(2, keys.Count);
Assert.Equal("2", keys[0]);
Assert.Equal("10", keys[1]);
}

[Theory]
[ClassData(typeof(OverrideGetChildKeysConfigurationProviders))]
public void ConfigurationGetChildrenNotOptimizedConfigurationProvider(IConfigurationProvider provider)
{
// Arrange
var dic2 = new Dictionary<string, string>()
{
{"key2", "val"},
};
var src1 = new FixedProviderConfigurationSource(provider);
var memConfigSrc2 = new MemoryConfigurationSource { InitialData = dic2 };
var configurationBuilder = new ConfigurationBuilder();
configurationBuilder.Add(src1);
configurationBuilder.Add(memConfigSrc2);
var configSorted = configurationBuilder.Build();
configurationBuilder = new ConfigurationBuilder();
configurationBuilder.Add(memConfigSrc2);
configurationBuilder.Add(src1);
var configNotSorted = configurationBuilder.Build();

// Act
var keysSorted = configSorted.GetChildren().Select(c => c.Key).ToList();
var keysNotSorted = configNotSorted.GetChildren().Select(c => c.Key).ToList();

// Assert
Assert.Equal(3, keysSorted.Count);
Assert.Equal(3, keysNotSorted.Count);

// The keys should be sorted by the 2nd IConfigurationProvider
// because it inherits from helper class ConfigurationProvider.
Assert.Equal("key1", keysSorted[0]);
Assert.Equal("key2", keysSorted[1]);
Assert.Equal("key3", keysSorted[2]);

// Actually here is a breaking change!
// previously the order is controlled by the last provider
// now the order is controalled in GetChildrenImplementation, which is always sorted
#if !NET7_0_OR_GREATER
Assert.Equal("key2", keysNotSorted[0]);
Assert.Equal("key3", keysNotSorted[1]);
Assert.Equal("key1", keysNotSorted[2]);
#endif
}

private class OverrideGetChildKeysConfigurationProviders : TheoryData<IConfigurationProvider>
{
public OverrideGetChildKeysConfigurationProviders()
{
Add(new InheritHelperClassButOverrideMethodGetChildKeysConfigurationProvider());
Add(new NotInheritHelperClassConfigurationProvider());
}

public static IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath)
{
if (parentPath != null)
{
throw new ArgumentException("Not support parentPath");
}

foreach (var key in earlierKeys)
{
yield return key;
}

// For the IConfigurationProvider implementation that not inherit from helper class ConfigurationProvider,
// the returned keys might not be sorted.
yield return "key3";
yield return "key1";
}
}

private class FixedProviderConfigurationSource : IConfigurationSource
{
private readonly IConfigurationProvider provider;
public FixedProviderConfigurationSource(IConfigurationProvider provider)
{
this.provider = provider;
}

public IConfigurationProvider Build(IConfigurationBuilder builder) => provider;
}

private class InheritHelperClassButOverrideMethodGetChildKeysConfigurationProvider : ConfigurationProvider
{
public override IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath)
=> OverrideGetChildKeysConfigurationProviders.GetChildKeys(earlierKeys, parentPath);
}

private class NotInheritHelperClassConfigurationProvider : IConfigurationProvider
{
public IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath)
=> OverrideGetChildKeysConfigurationProviders.GetChildKeys(earlierKeys, parentPath);

public Primitives.IChangeToken GetReloadToken() => new ConfigurationReloadToken();

public void Load()
{
}

public void Set(string key, string? value)
{
}

public bool TryGet(string key, out string? value)
{
value = "val";
return true;
}
}
}
}