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

ChainedConfigurationProvider.GetChildKeys is inefficient #62510

Closed
Tracked by #64015
davidfowl opened this issue Dec 8, 2021 · 8 comments · Fixed by #67186
Closed
Tracked by #64015

ChainedConfigurationProvider.GetChildKeys is inefficient #62510

davidfowl opened this issue Dec 8, 2021 · 8 comments · Fixed by #67186
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Dec 8, 2021

Description

ChainedConfigurationProvider.GetChildKeys ends up splitting strings which forces an array allocation even if there are no keys with segments. This has shown up on a couple of performance profiles internally and we should improve it.

Regression?

No

Data

Ask me

Analysis

Ask me

@davidfowl davidfowl added the tenet-performance Performance related issue label Dec 8, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Configuration untriaged New issue has not been triaged by the area owner labels Dec 8, 2021
@ghost
Copy link

ghost commented Dec 8, 2021

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

Issue Details

Description

ChainedConfigurationProvider.GetChildKeys ends up splitting strings which forces an array allocation even if there are no keys with segments. This has shown up on a couple of performance profiles internally and we should improve it.

Regression?

No

Data

Ask me

Analysis

Ask me

Author: davidfowl
Assignees: -
Labels:

tenet-performance, untriaged, area-Extensions-Configuration

Milestone: -

@gfoidl
Copy link
Member

gfoidl commented Dec 8, 2021

I can look into this (next week).

Seems there's some potential for improvement.
Use of

private static readonly string[] _keyDelimiterArray = new[] { ConfigurationPath.KeyDelimiter };
can also be simplified (it's all static readonly for :).

@vcsjones
Copy link
Member

vcsjones commented Dec 8, 2021

If this shows up on profiles, then I think you could do this allocation free entirely.

The below code is entirely untested and for illustration purposes only 😄. It might be slower for all I know, but it doesn't allocate strings to split. This has a slight behavior change in what is returned when the segments aren't equal, but it conforms to the contract of Compare. If we want to retain identical behavior, it's possible, but would result in doing unnecessary splitting.

namespace Microsoft.Extensions.Configuration
{
    /// <summary>
    /// IComparer implementation used to order configuration keys.
    /// </summary>
    public class ConfigurationKeyComparer : IComparer<string>
    {
        /// <summary>
        /// The default instance.
        /// </summary>
        public static ConfigurationKeyComparer Instance { get; } = new ConfigurationKeyComparer();

        /// <summary>A comparer delegate with the default instance.</summary>
        internal static Comparison<string> Comparison { get; } = Instance.Compare;

        public static void Split(
            ReadOnlySpan<char> str,
            char delimiter,
            out ReadOnlySpan<char> next,
            out ReadOnlySpan<char> remaining,
            bool skipEmpty = true)
        {
            while (true)
            {
                int location = str.IndexOf(delimiter);

                if (location == -1)
                {
                    next = str;
                    remaining = default;
                    return;
                }
                else if (location == 0 && skipEmpty)
                {
                    str = str[1..];
                    continue;
                }
                else
                {
                    next = str[..location];
                    remaining = str[(location + 1)..];
                    return;
                }
            }
        }

        public int Compare(string? x, string? y)
        {
            ReadOnlySpan<char> xRemaining = x;
            ReadOnlySpan<char> yRemaining = y;

            do
            {
                Split(xRemaining, ':', out ReadOnlySpan<char> xPart, out xRemaining);
                Split(yRemaining, ':', out ReadOnlySpan<char> yPart, out yRemaining);

                bool xIsInt = int.TryParse(xPart, out int value1);
                bool yIsInt = int.TryParse(yPart, out int value2);

                int result;

                if (!xIsInt && !yIsInt)
                {
                    // Both are strings
                    result = MemoryExtensions.CompareTo(xPart, yPart, StringComparison.OrdinalIgnoreCase);
                }
                else if (xIsInt && yIsInt)
                {
                    // Both are int
                    result = value1 - value2;
                }
                else
                {
                    // Only one of them is int
                    result = xIsInt ? -1 : 1;
                }

                if (result != 0)
                {
                    // One of them is different
                    return result;
                }
            }
            while (!xRemaining.IsEmpty && !yRemaining.IsEmpty);

            return xRemaining.Length - yRemaining.Length;
        }
    }
}

@Wraith2
Copy link
Contributor

Wraith2 commented Dec 8, 2021

I was going to tinker with using the existing StringSegment Tokenizer in the extensions Primitive namespace. What would be useful is some example input strings to run against.

@SteveDunn
Copy link
Contributor

I'm happy to look at this one. Is there a project with a suitably large set of config to baseline?

@maryamariyan
Copy link
Member

Thanks @SteveDunn this issue is currently in progress.

@SteveDunn
Copy link
Contributor

Thanks @SteveDunn this issue is currently in progress.

Sorry, I didn't see a branch/PR

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 26, 2022
@maryamariyan
Copy link
Member

Sorry, I didn't see a branch/PR

@SteveDunn no worries, just added the PR here in #67186

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 31, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants