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

Implement Include/ThenInclude as adapter #188

Merged
merged 6 commits into from
Dec 21, 2021

Conversation

devbased
Copy link
Contributor

@devbased devbased commented Dec 19, 2021

Closes #187.

Summary

Query should be built by ORM (EfCore and EF6 in our case). This work uses data from IncludeExpressionInfo to call public API of specified libraries instead of building query manually.

Part with EF6 is not done yet

@fiseni
Copy link
Collaborator

fiseni commented Dec 19, 2021

Hey @ardalis,

If you don't have any comments here, I'll merge this. In the end, we decided to be an opt-in feature, so there is no high risk of implementing it.

…ollection) with ThenInclude reference|collection; add comments to show what constructed expression trees represent
@fiseni
Copy link
Collaborator

fiseni commented Dec 20, 2021

Hey @devbased,

The tests are passing and everything seems working fine with caching enabled. But, I'm somewhat uncomfortable with benchmark results. We're doing additional work on top of EF, and we just can't be faster than EF. What we're missing here?

When you compile the lambda for the proxy delegates, perhaps it compiles recursively the inner LambdaExpression too (the query expression)? We're passing the compiled version? 🤔 That would not be good.

Benchmark results for 1.000.000 iterations:

Method Mean Error StdDev Gen 0 Allocated
EFIncludeExpression 4.600 s 0.0849 s 0.1441 s 502000.0000 3 GB
SpecIncludeExpression 5.619 s 0.1122 s 0.1292 s 585000.0000 3 GB
SpecIncludeExpressionCached 3.642 s 0.0724 s 0.1015 s 415000.0000 2 GB

@fiseni fiseni self-requested a review December 20, 2021 14:34
@fiseni
Copy link
Collaborator

fiseni commented Dec 20, 2021

Ok, the issue was in the benchmark tests. For the EF direct approach, the expressions were created on each iteration, while the spec versions have the expression stored as a state. Once we fix this, then we get much more accurate and reasonable results. Actually, now we can see how much slower we are compared with direct EF usage 😞

Benchmark results for 100.000 iterations:

Method Mean Error StdDev Gen 0 Allocated
EFIncludeExpression 273.9 ms 1.10 ms 0.86 ms 31500.0000 189 MB
SpecIncludeExpression 642.3 ms 7.26 ms 6.79 ms 69000.0000 417 MB
SpecIncludeExpressionCached 377.7 ms 7.44 ms 6.96 ms 44000.0000 266 MB

The test code:

namespace Specification.Benchmark
{
    [MemoryDiagnoser]
    public class IncludeBenchmark
    {
        private readonly int max = 100000;
        private readonly SpecificationEvaluator evaluator = SpecificationEvaluator.Default;
        private readonly SpecificationEvaluator evaluatorCached = SpecificationEvaluator.Cached;
        private readonly Specification<Store> specInclude = new StoreIncludeProductsSpec();

        private readonly IQueryable<Store> Stores;

        public IncludeBenchmark()
        {
            Stores = new BenchmarkDbContext().Stores.AsQueryable();
        }

        [Benchmark]
        public void EFIncludeExpression()
        {
            Expression<Func<Store, IEnumerable<Product>>> IncludeExpr = x => x.Products;
            Expression<Func<Product, CustomFields>> ThenIncludeExpr = x => x.CustomFields;

            for (int i = 0; i < max; i++)
            {
                _ = Stores.Include(IncludeExpr).ThenInclude(ThenIncludeExpr);
            }
        }

        [Benchmark]
        public void SpecIncludeExpression()
        {
            for (int i = 0; i < max; i++)
            {
                _ = evaluator.GetQuery(Stores, specInclude);
            }
        }

        [Benchmark]
        public void SpecIncludeExpressionCached()
        {
            for (int i = 0; i < max; i++)
            {
                _ = evaluatorCached.GetQuery(Stores, specInclude);
            }
        }
    }

    public class BenchmarkDbContext : DbContext
    {
        public DbSet<Store> Stores { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            base.OnConfiguring(optionsBuilder);

            optionsBuilder.UseSqlServer("Server=(localdb)\\mssqllocaldb;Integrated Security=SSPI;Initial Catalog=SpecificationEFTestsDB;ConnectRetryCount=0");
        }
    }

    public class StoreIncludeProductsSpec : Specification<Store>
    {
        public StoreIncludeProductsSpec()
        {
            Query.Include(x => x.Products).ThenInclude(x => x.CustomFields);
        }
    }

    public class Store
    {
        public int Id { get; set; }
        public IEnumerable<Product> Products { get; set; }
    }

    public class Product
    {
        public int Id { get; set; }
        public CustomFields CustomFields { get; set; }
    }

    public class CustomFields
    {
        public int Id { get; set; }
        public string CustomText1 { get; set; }
        public string CustomText2 { get; set; }
    }
}

…reByIdIncludeCompanyAndCountryAndStoresForCompanySpec' test for more complex include scenario; add it to EF6
@fiseni fiseni merged commit 098eaee into ardalis:main Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rethinking Include implementation
2 participants