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

Options generator perf improvements #91073

Closed
stephentoub opened this issue Aug 24, 2023 · 4 comments
Closed

Options generator perf improvements #91073

stephentoub opened this issue Aug 24, 2023 · 4 comments
Labels
area-Extensions-Options source-generator Indicates an issue with a source generator feature tenet-performance Performance related issue
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Aug 24, 2023

The options generator is emitting code like this:

            var baseName = (string.IsNullOrEmpty(name) ? "FirstModel" : name) + ".";
            var builder = new global::Microsoft.Extensions.Options.ValidateOptionsResultBuilder();
            var context = new global::System.ComponentModel.DataAnnotations.ValidationContext(options);
            var validationResults = new global::System.Collections.Generic.List<global::System.ComponentModel.DataAnnotations.ValidationResult>();
            var validationAttributes = new global::System.Collections.Generic.List<global::System.ComponentModel.DataAnnotations.ValidationAttribute>(1);

            context.MemberName = "P1";
            context.DisplayName = baseName + "P1";
            validationAttributes.Add(global::__OptionValidationStaticInstances.__Attributes.A3);
            if (!global::System.ComponentModel.DataAnnotations.Validator.TryValidateValue(options.P1!, context, validationResults, validationAttributes))
            {
                builder.AddResults(validationResults);
            }

            context.MemberName = "P2";
            context.DisplayName = baseName + "P2";
            validationResults.Clear();
            validationAttributes.Clear();
            validationAttributes.Add(global::__OptionValidationStaticInstances.__Attributes.A4);
            if (!global::System.ComponentModel.DataAnnotations.Validator.TryValidateValue(options.P2!, context, validationResults, validationAttributes))
            {
                builder.AddResults(validationResults);
            }

            return builder.Build();
  1. There's a concatenation string allocation that results as part of creating baseName, even in the common case where name is null and both things being concatenated are literals. There's then another concatenation allocation per member, as the implementation tacks on a literal suffix. Instead of var baseName = (string.IsNullOrEmpty(name) ? "FirstModel" : name) + ".";, that line should be deleted, the each display name creation should instead be like: context.DisplayName = string.IsNullOrEmpty(name) ? "FirstModel.P2" : name + ".P2";. That way, when the name is null or empty, there are zero string allocations, and when the name is non-null and non-empty, it saves the one intermediate allocation.
  2. ValidateOptionsResultBuilder is a class and is being allocated even when there aren't any validation errors. We should instead lazily allocate it only when one of the TryValidateValue calls returns false.
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 24, 2023
@ghost
Copy link

ghost commented Aug 24, 2023

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

Issue Details

The options generator is emitting code like this:

            var baseName = (string.IsNullOrEmpty(name) ? "FirstModel" : name) + ".";
            var builder = new global::Microsoft.Extensions.Options.ValidateOptionsResultBuilder();
            var context = new global::System.ComponentModel.DataAnnotations.ValidationContext(options);
            var validationResults = new global::System.Collections.Generic.List<global::System.ComponentModel.DataAnnotations.ValidationResult>();
            var validationAttributes = new global::System.Collections.Generic.List<global::System.ComponentModel.DataAnnotations.ValidationAttribute>(1);

            context.MemberName = "P1";
            context.DisplayName = baseName + "P1";
            validationAttributes.Add(global::__OptionValidationStaticInstances.__Attributes.A3);
            if (!global::System.ComponentModel.DataAnnotations.Validator.TryValidateValue(options.P1!, context, validationResults, validationAttributes))
            {
                builder.AddResults(validationResults);
            }

            context.MemberName = "P2";
            context.DisplayName = baseName + "P2";
            validationResults.Clear();
            validationAttributes.Clear();
            validationAttributes.Add(global::__OptionValidationStaticInstances.__Attributes.A4);
            if (!global::System.ComponentModel.DataAnnotations.Validator.TryValidateValue(options.P2!, context, validationResults, validationAttributes))
            {
                builder.AddResults(validationResults);
            }

            return builder.Build();
  1. There's a concatenation string allocation that results as part of creating baseName, even in the common case where name is null and both things being concatenated are literals. There's then another concatenation allocation per member, as the implementation tacks on a literal suffix. Instead of var baseName = (string.IsNullOrEmpty(name) ? "FirstModel" : name) + ".";, that line should be deleted, the each display name creation should instead be like: context.DisplayName = string.IsNullOrEmpty(name) ? "FirstModel.P2" : name + ".P2";`. That way, when the name is null or empty, there are zero string allocations, and when the name is non-null and non-empty, it saves the one intermediate allocation.
  2. ValidateOptionsResultBuilder is a class and is being allocated even when there aren't any validation errors. We should instead lazily allocate it only when one of the TryValidateValue calls returns false.
Author: stephentoub
Assignees: -
Labels:

area-System.ComponentModel.DataAnnotations, untriaged

Milestone: -

@ghost
Copy link

ghost commented Aug 24, 2023

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

Issue Details

The options generator is emitting code like this:

            var baseName = (string.IsNullOrEmpty(name) ? "FirstModel" : name) + ".";
            var builder = new global::Microsoft.Extensions.Options.ValidateOptionsResultBuilder();
            var context = new global::System.ComponentModel.DataAnnotations.ValidationContext(options);
            var validationResults = new global::System.Collections.Generic.List<global::System.ComponentModel.DataAnnotations.ValidationResult>();
            var validationAttributes = new global::System.Collections.Generic.List<global::System.ComponentModel.DataAnnotations.ValidationAttribute>(1);

            context.MemberName = "P1";
            context.DisplayName = baseName + "P1";
            validationAttributes.Add(global::__OptionValidationStaticInstances.__Attributes.A3);
            if (!global::System.ComponentModel.DataAnnotations.Validator.TryValidateValue(options.P1!, context, validationResults, validationAttributes))
            {
                builder.AddResults(validationResults);
            }

            context.MemberName = "P2";
            context.DisplayName = baseName + "P2";
            validationResults.Clear();
            validationAttributes.Clear();
            validationAttributes.Add(global::__OptionValidationStaticInstances.__Attributes.A4);
            if (!global::System.ComponentModel.DataAnnotations.Validator.TryValidateValue(options.P2!, context, validationResults, validationAttributes))
            {
                builder.AddResults(validationResults);
            }

            return builder.Build();
  1. There's a concatenation string allocation that results as part of creating baseName, even in the common case where name is null and both things being concatenated are literals. There's then another concatenation allocation per member, as the implementation tacks on a literal suffix. Instead of var baseName = (string.IsNullOrEmpty(name) ? "FirstModel" : name) + ".";, that line should be deleted, the each display name creation should instead be like: context.DisplayName = string.IsNullOrEmpty(name) ? "FirstModel.P2" : name + ".P2";. That way, when the name is null or empty, there are zero string allocations, and when the name is non-null and non-empty, it saves the one intermediate allocation.
  2. ValidateOptionsResultBuilder is a class and is being allocated even when there aren't any validation errors. We should instead lazily allocate it only when one of the TryValidateValue calls returns false.
Author: stephentoub
Assignees: -
Labels:

untriaged, area-Extensions-Options

Milestone: -

@layomia layomia added the source-generator Indicates an issue with a source generator feature label Aug 24, 2023
@layomia
Copy link
Contributor

layomia commented Aug 24, 2023

cc @tarekgh.

@layomia layomia added the tenet-performance Performance related issue label Aug 24, 2023
@tarekgh tarekgh added this to the 9.0.0 milestone Aug 24, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 24, 2023
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Aug 31, 2023
@tarekgh
Copy link
Member

tarekgh commented Sep 1, 2023

This is fixed by #91432

@tarekgh tarekgh closed this as completed Sep 1, 2023
@tarekgh tarekgh modified the milestones: 9.0.0, 8.0.0 Sep 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Options source-generator Indicates an issue with a source generator feature tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

3 participants