dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.7k stars 3.17k forks source link

Performance issue with eager loading and lazy loading #12451

Closed bmitchenko closed 6 years ago

bmitchenko commented 6 years ago

Consider the following example:

// 1
var cars = db.CarList
    .Include(x => x.Model)
    .Take(1000)
    .ToList();

// 2
foreach (var car in cars)
{
    var model = car.Model;
}

After the data was loaded (1), the loop (2) took a few milliseconds to complete without lazy loading. When lazy loading is enabled with .UseLazyLoadingProxies() the loop is pretty slow and there are many messages in the log:

DetectChanges starting for 'OrderDbContext'.
DetectChanges completed for 'OrderDbContext'.

Even if I repeat the loop (2) again, it still works very slowly.

Further technical details

EF Core version: 2.1 Database Provider: Npgsql.EntityFrameworkCore.PostgreSQL Operating system: Windows 10 IDE: Visual Studio 2017 15.7.4

khalidabuhakmeh commented 6 years ago

What does slow mean? Could you use a tool like BenchmarkDotNet to quantify what you are seeing?

bmitchenko commented 6 years ago

The loop took 0.05 ms without lazy loading 22 sec with lazy loading

khalidabuhakmeh commented 6 years ago

Have you tried the same test against SQL Server? It may help isolate the issue to EF Core 2.1 vs. the underlying database provider.

bmitchenko commented 6 years ago

Tried to reproduce this issue with SQL Server and got almost the same result.

Here is what my entities look like:

public class OrderDbContext : DbContext
{
    public DbSet<Category> Categories { get; set; }
    public DbSet<Product> Products { get; set; }

     public OrderDbContext(DbContextOptions<OrderDbContext> options)
         : base(options)
     {
     }
 }

public class Category
{
    public int Id { get; set; }
    public string Name { get; set; }
}

public class Product
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int CategoryId { get; set; }
    public virtual Category Category { get; set; }
}

And here is code that I tried with SQL Server:

var products = db.Products
    .Include(x => x.Category)
    .Take(1000)
    .ToList();

foreach (var product in products)
{
    var category = product.Category;
}
khalidabuhakmeh commented 6 years ago

I see you have a Take(1000). How big is your test dataset? Do you have a csv or json dump of your sample?

bmitchenko commented 6 years ago

Unfortunately, I can not provide the data, because I tested on the production server. But I'm sure that any database with the "one-to-many" relation is suitable for reproducing this problem. Performance degradation is visible even on 10 rows, as every time I access preloaded navigation property it takes some time. And it almost instant without lazy loading.

ajcvickers commented 6 years ago

@bmitchenko Thanks for reporting this. Looks like a bug. Can you try locally turning off automatic DetectChanges:

try
{
    db.ChangeTracker.AutoDetectChangesEnabled = false;

    foreach (var car in cars)
    {
        var model = car.Model;
    }
}
finally
{
    db.ChangeTracker.AutoDetectChangesEnabled = true;
}
bmitchenko commented 6 years ago

After disabling automatic change detection, the problem disappears.

nomad3k commented 6 years ago

I can confirm that I’m also seeing this issue in my code. I have a dataset of 7500 records, and this causes a ~600ms delay per record for me. Meaning my query would take about 75 minutes to complete.

I can’t give specifics but it looks something like this.

var items = await context.Things.Include(x => x.Parents).ToArrayAsync();
var rootItems = items.Where(x => !x.Parents.Any());
return ConvertToTree(rootItems);

I have implemented the workaround above, and the query now runs in seconds, but now have code with comments referencing this bug that will need to be removed when this is resolved. :)

ChristophvdF commented 6 years ago

Hi,

I encounter the same issue with the lazy loader but my setup is a bit different. All values I need are eager loaded

            var project = await DbContext.Projects
                .Include(p => p.Customer)
                .Include(p => p.Operator)
                .Include(p => p.ProjectManager)
                .Include(p => p.TechnicalManager)
                .Include(p => p.Subprojects)
                    .ThenInclude(s => s.ProductData)
                        .ThenInclude(o => o.TechnicalData)
                .Include(p => p.Subprojects)
                    .ThenInclude(s => s.SapData)
                .SingleOrDefaultAsync(p => p.Id == id);

            if (project == null)
            {
                return null;
            }

            project.Subprojects = project.Subprojects
                    .OrderBy(sp => sp.SubprojectNo)
                    .ThenBy(sp => sp.Variant)
                    .ThenBy(sp => sp.PositionNo)
                    .ThenBy(sp => sp.SubprojectType)
                    .ToList();

            return project;

When I wrap the whole block like

        try
        {
            DbContext.ChangeTracker.AutoDetectChangesEnabled = false;

            var project = await DbContext.Projects
                .Include(p => p.Customer)
                .Include(p => p.Operator)
                .Include(p => p.ProjectManager)
                .Include(p => p.TechnicalManager)
                .Include(p => p.Subprojects)
                    .ThenInclude(s => s.ProductData)
                        .ThenInclude(o => o.TechnicalData)
                .Include(p => p.Subprojects)
                    .ThenInclude(s => s.SapData)
                .SingleOrDefaultAsync(p => p.Id == id);

            if (project == null)
            {
                return null;
            }

            project.Subprojects = project.Subprojects
                    .OrderBy(sp => sp.SubprojectNo)
                    .ThenBy(sp => sp.Variant)
                    .ThenBy(sp => sp.PositionNo)
                    .ThenBy(sp => sp.SubprojectType)
                    .ToList();

            return project;
        }
        finally
        {
            DbContext.ChangeTracker.AutoDetectChangesEnabled = true;
        }

When I disable AutoDetectChangesEnabled without re enabling it the call is done in ms but when I enable it again the call takes about 8 min.

It seams that I have to disable the AutoDetectChangesEnabled and reenable it outside of the method I am calling.

ajcvickers commented 6 years ago

@ChristophvdF Lazy-loading needs to be disabled where the navigation properties are being accessed.

ChristophvdF commented 6 years ago

@ajcvickers Thx for the clarification.

jonathanmoffatt commented 6 years ago

Any update on when a fix for this may be available? It makes lazy loading unusable for much of our application.

ajcvickers commented 6 years ago

@jonathanmoffatt It's in the 2.2 milestone, which means it is planned for the 2.2 release.

mryabets commented 6 years ago

Will there be a fix for 2.1 or 2.2 Preview?

ajcvickers commented 6 years ago

@mryabets It will be in 2.2 preview 3.

optiks commented 5 years ago

If you're injecting ILazyLoader in 2.1, a workaround is to create a custom ILazyLoader that wraps the Load in AutoDetectChangesEnabled = true/false. See https://gist.github.com/optiks/482f5c9a6f1dc56d58d1ea8a86c9b925.

alextretij commented 5 years ago

Can I replace ILazyLoader for all existing or new DBContexts? I can do it using this command

services
    .AddDbContext<MyContext>(b => b
        .ReplaceService<ILazyLoader , CustomLazyLoader >());

But I want to replace service for all new created DBContexts.

optiks commented 5 years ago

@alextretij, what's the actual issue you're having?

alextretij commented 5 years ago

I use NopCommerce. NopCommerce has internal DBContext and many external plugins use own DBContexts. I can't add ".ReplaceService<ILazyLoader , CustomLazyLoader >());" for each DBContext.

I thought to add external plugin, run it the first and overwrite ILazyLoader for all new DBContexts. I read this article https://blog.oneunicorn.com/2016/10/27/dependency-injection-in-ef-core-1-1/

I think how to change service in the service provider to DbContext?

alextretij commented 5 years ago

what's the actual issue you're having? I want to solve this issue for EF 2.1 without changing the kernel of nopcommerce (or any external plugins). I hope to use this solution for solving the performance issue for any sites with EF Core 2.1.

amavzyutov commented 5 years ago

Hi. Tests in code below was ok in 2.1.4 and fails in 2.2.0-preview3-35497. Is it ok?

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;
using Microsoft.EntityFrameworkCore;
using Xunit;

namespace EFCoreLazyLoadingTest
{
    public class EntityOne
    {
        [Key]
        public int Id { get; set; }
        public virtual ICollection<EntityTwo> EntityTwoCollection { get; set; }
    }
    public class EntityTwo
    {
        [Key]
        public int Id { get; set; }

        [ForeignKey(nameof(EntityOneId))]
        public virtual EntityOne EntityOne { get; set; }

        public int EntityOneId { get; set; }
    }

    public class MyDbContext : DbContext
    {
        public DbSet<EntityOne> EntityOnes { get; set; }
        public DbSet<EntityTwo> EntityTwos { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
            => optionsBuilder.UseLazyLoadingProxies().UseInMemoryDatabase("testDb");
    }

    public class Tests
    {
        [Fact]
        public void TestCollectionRelationById()
        {
            using (var context = new MyDbContext())
            {
                var newEntityOne = context.CreateProxy<EntityOne>();  
                newEntityOne.Id = 1;
                context.EntityOnes.Add(newEntityOne);

                var newEntityTwo = context.CreateProxy<EntityTwo>(); 
                context.EntityTwos.Add(newEntityTwo);
                newEntityTwo.EntityOneId = 1;
                Assert.NotEmpty(newEntityOne.EntityTwoCollection);
            }
        }

        [Fact]
        public void TestCollectionRelationByNavigationProperty()
        {
            using (var context = new MyDbContext())
            {
                var newEntityOne = context.CreateProxy<EntityOne>(); 
                newEntityOne.Id = 1;
                context.EntityOnes.Add(newEntityOne);

                var newEntityTwo = context.CreateProxy<EntityTwo>();  
                context.EntityTwos.Add(newEntityTwo);
                newEntityTwo.EntityOne = newEntityOne;
                Assert.True(newEntityOne.EntityTwoCollection.Count!=0);
            }
        }

        [Fact]
        public void TestRelationByBackNavigationProperty()
        {
            using (var context = new MyDbContext())
            {
                var newEntityOne = context.CreateProxy<EntityOne>();  
                newEntityOne.Id = 1;
                context.EntityOnes.Add(newEntityOne);

                var newEntityTwo = context.CreateProxy<EntityTwo>(); 
                newEntityOne.EntityTwoCollection.Add(newEntityTwo);
                Assert.NotNull(newEntityTwo.EntityOne);
            }
        }

        [Fact]
        public void TestRelationById()
        {
            using (var context = new MyDbContext())
            {
                var newEntityOne = context.CreateProxy<EntityOne>();  
                newEntityOne.Id = 1;
                context.EntityOnes.Add(newEntityOne);

                var newEntityTwo = context.CreateProxy<EntityTwo>(); 
                context.EntityTwos.Add(newEntityTwo);
                newEntityTwo.EntityOneId = 1;
                Assert.NotNull(newEntityTwo.EntityOne);
            }
        }
    }
}
ajcvickers commented 5 years ago

@amavzyutov Generally speaking, EF needs to get a chance to detect changes made to the tracked entities before it will be aware of such changes. (The posts above are for EF6, but basically the same thing applies to EF Core.) In 2.1, lazy-loading was accidentally triggering DetectChanges, and hence you were seeing different results there.