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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Auto properties on large record types causes severe performance degradation #1432

Open
3 tasks
putnap opened this issue Dec 21, 2023 · 4 comments
Open
3 tasks
Labels

Comments

@putnap
Copy link

putnap commented Dec 21, 2023

Describe the Bug

It looks like record type with constructor is a cause for performance degradation, which can be worked around by using OmitAutoProperties() or by customizing each property using Without().

Scenario

In our project we are using rather wide and deeply nested structure as our main DTO and we are using xUnit, AutoFixture and Moq to write our tests. We have about 120 unit tests a lot of them create this large DTO using AutoMoqDataAttribute. The suite of tests in our CI consistently run for around 25 minutes, which is horrible.
I started exploring why that is and I had a strong suspicion that our large DTO is causing this, because tests without it completed very quickly, I tried then tried reducing the amount of auto properties on the structure to only header values and tests started to run quick, but I still had all the values automatically generated. I was happy I found a workaround that helps us in significant way, but at the same time I am concerned why this is happening.

I've created a repository which has minimum setup needed to reproduce the issue and basic explanation is this:

  • Record types are created by using it's constructor
  • Customizations that use OmitAutoProperties() or Without() don't work on constructors for record types
  • Even by omitting auto properties on record type, the record still has all properties set (assuming via constructor)
  • When omitting Auto properties in our setup tests started executing at least ~10 times faster (sample project executes tests ~40 times faster)

I've created a customization that checks if property is record type property by assuming that it has CompilerGeneratedAttribute.

Expected Behavior

By using OmitAutoProperties() or Without() it's expected that record type will not have a value automatically set.

Tasks

  • Explain what is going on
  • Updated all AutoFixture and related libraries
  • Confirmed in support forums if behavior is expected

Immediate question

I am fine having this workaround for now, but maybe someone can confirm that it won't cause any other issues for running tests?

Thanks!

@putnap putnap added the bug label Dec 21, 2023
@aivascu aivascu added the triage Something that's being investigated label Dec 21, 2023
@aivascu
Copy link
Member

aivascu commented Dec 21, 2023

Hi @putnap,
Thank you for raising this issue. What I think the issue is, it's that records create classes that do not properly encapsulate data, when looked at from the perspective of reflection (see below).

AutoFixture currently looks at the list of constructors takes the least demanding public constructor and uses it to initialize the data, then it looks at your other members and assumes that since you exposed a public setter you need it initialized from outside the constructor, so it does just that. This is why you're observing the behavior that you described.

I'm currently working on an API for easier customization of record-like structures (customizing constructor parameters, selecting constructors, etc), but it's still in early development.
Feature requests like this have been raised before, but as far as I remember this wasn't implemented before because there is no static way to define constructor parameters like you would with members (ie. .With(x => x.Member, "MemberValue") ).

// Example record
record Customer(string FirstName, string LastName);

// Equivalent class generated in IL
class Customer
{
    public Customer(string FirstName, string LastName)
    {
        this.FirstName = FirstName;
        this.LastName = LastName;
    }

    public string FirstName { get; set; }
    public string FirstName { get; set; }
}

@putnap
Copy link
Author

putnap commented Dec 21, 2023

Thanks for a quick reply!

I guess there are two topics - customizing via record constructors feature and performance issues.
Right now I want to understand why performance hit is so severe.

Also, I believe that class you mention is not equivalent, since record types are immutable by default and generated properties don't have public setters, only init setters.

    public string FirstName { get; init; }
    public string LastName { get; init; }

And I would like some peace of mind that my ICustomization with CompilerGeneratedAttribute filter in this file is a good enough general workaround for now. I don't fully grasp all repercussions of using new OmitSpecimen(); so dramatically.

@aivascu
Copy link
Member

aivascu commented Dec 21, 2023

About the init setters you're kinda correct. It does generate the equivalent IL for init for positional records, however the difference between a public setter and init is an attribute set on the return value of the setter method, which AutoFixture at the moment doesn't know exists. Everything else in terms of accessibility modifiers is exactly the same as a public setter.

This said I don't think the customization in your sample is doing anything else than ignoring all properties.
A more correct way to ignore all redundant property setter is to explicitly account for them.

Here is a gist that shows an example, how to ignore specifically those properties that are init only and constructor initialized
https://gist.github.com/aivascu/739f2a845eac587ca4df11cd9c5f7d31

Expand for IL examples
// usual public setter
  .property instance string Name()
  {
    .get instance string TestProject.ClassA::get_Name()
    .set instance void TestProject.ClassA::set_Name(string)
  } // end of property ClassA::Name

// positional record property
  .property instance string Name()
  {
    .get instance string TestProject.ClassB::get_Name()
    .set instance void modreq ([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) TestProject.ClassB::set_Name(string)
  } // end of property ClassB::Name

// init-only property
  .property instance string Name()
  {
    .get instance string TestProject.ClassC::get_Name()
    .set instance void modreq ([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) TestProject.ClassC::set_Name(string)
  } // end of property ClassC::Name

@putnap
Copy link
Author

putnap commented Dec 21, 2023

Thanks, I'll use the workaround you suggested, seems to work!

@aivascu aivascu removed the triage Something that's being investigated label Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants