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

Config generator emits lambdas for error path that force allocation on success path #90971

Closed
stephentoub opened this issue Aug 23, 2023 · 2 comments · Fixed by #100257
Closed
Labels
area-Extensions-Configuration help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged Priority:2 Work that is important, but not critical for the release source-generator Indicates an issue with a source generator feature tenet-performance Performance related issue
Milestone

Comments

@stephentoub
Copy link
Member

Consider a method like:

internal class C1
{
    public int Value { get; set; }

    public static void M(IConfiguration configuration, C1 c) => configuration.Bind(c);
}

That results in this BindCore method being emitted:

        public static void BindCore(IConfiguration configuration, ref C1 obj, BinderOptions? binderOptions)
        {
            ValidateConfigurationKeys(typeof(C1), s_configKeys_C1, configuration, binderOptions);

            if (configuration["Value"] is string value15)
            {
                obj.Value = ParseInt(value15, () => configuration.GetSection("Value").Path);
            }
        }

That ParseInt call closes over the configuration parameter, which means the compiler will lift the configuration parameter to a display class and generate code like this:

public static void BindCore(IConfiguration configuration, ref C1 obj, [Nullable(2)] BinderOptions binderOptions)
{
	<>c__DisplayClass15_0 <>c__DisplayClass15_ = new <>c__DisplayClass15_0();
	<>c__DisplayClass15_.configuration = configuration;
	ValidateConfigurationKeys(typeof(C1), s_configKeys_C1, <>c__DisplayClass15_.configuration, binderOptions);
	string value22 = <>c__DisplayClass15_.configuration["Value"];
	if (value22 != null)
	{
		obj.Value = ParseInt(value22, new Func<string>(<>c__DisplayClass15_.<BindCore>b__0));
	}
}

That means we're allocating a <>c__DisplayClass15_0 object on all invocation, even though that lambda causing the closure is only used by ParseInt when parsing fails and an exception is thrown and caught.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 23, 2023
@ghost
Copy link

ghost commented Aug 23, 2023

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

Issue Details

Consider a method like:

internal class C1
{
    public int Value { get; set; }

    public static void M(IConfiguration configuration, C1 c) => configuration.Bind(c);
}

That results in this BindCore method being emitted:

        public static void BindCore(IConfiguration configuration, ref C1 obj, BinderOptions? binderOptions)
        {
            ValidateConfigurationKeys(typeof(C1), s_configKeys_C1, configuration, binderOptions);

            if (configuration["Value"] is string value15)
            {
                obj.Value = ParseInt(value15, () => configuration.GetSection("Value").Path);
            }
        }

That ParseInt call closes over the configuration parameter, which means the compiler will lift the configuration parameter to a display class and generate code like this:

public static void BindCore(IConfiguration configuration, ref C1 obj, [Nullable(2)] BinderOptions binderOptions)
{
	<>c__DisplayClass15_0 <>c__DisplayClass15_ = new <>c__DisplayClass15_0();
	<>c__DisplayClass15_.configuration = configuration;
	ValidateConfigurationKeys(typeof(C1), s_configKeys_C1, <>c__DisplayClass15_.configuration, binderOptions);
	string value22 = <>c__DisplayClass15_.configuration["Value"];
	if (value22 != null)
	{
		obj.Value = ParseInt(value22, new Func<string>(<>c__DisplayClass15_.<BindCore>b__0));
	}
}

That means we're allocating a <>c__DisplayClass15_0 object on all invocation, even though that lambda causing the closure is only used by ParseInt when parsing fails and an exception is thrown and caught.

Author: stephentoub
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@layomia layomia added this to the 8.0.0 milestone Aug 23, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 23, 2023
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Aug 23, 2023
@layomia layomia self-assigned this Aug 23, 2023
@layomia layomia added source-generator Indicates an issue with a source generator feature tenet-performance Performance related issue labels Aug 23, 2023
@ericstj ericstj added the Priority:2 Work that is important, but not critical for the release label Sep 5, 2023
@ericstj ericstj modified the milestones: 8.0.0, 9.0.0 Sep 20, 2023
@ericstj
Copy link
Member

ericstj commented Sep 20, 2023

Moving this improvement to 9.0.0

@layomia layomia added the help wanted [up-for-grabs] Good issue for external contributors label Sep 25, 2023
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Mar 25, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged Priority:2 Work that is important, but not critical for the release source-generator Indicates an issue with a source generator feature tenet-performance Performance related issue
Projects
None yet
3 participants