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

EF Core 2.1 performance issue #3419

Closed
AndreiMaz opened this issue Nov 19, 2018 · 11 comments
Closed

EF Core 2.1 performance issue #3419

AndreiMaz opened this issue Nov 19, 2018 · 11 comments
Assignees
Milestone

Comments

@AndreiMaz
Copy link
Member

AndreiMaz commented Nov 19, 2018

nopCommerce version: 4.10

A critical performance issue was found in EF Core 2.1. More info at dotnet/efcore#12451

We've implemented a workaround at 57bb9a7 but waiting for the official fix from EF Core team

@AndreiMaz AndreiMaz added this to the Version 4.20 milestone Nov 19, 2018
@AndreiMaz AndreiMaz self-assigned this Nov 19, 2018
@Leftyx
Copy link

Leftyx commented Nov 19, 2018

@AndreiMaz The biggest offender in my implementation is the use of the caching system.

We have 340 categories and doing something like this:

_categoryService.GetAllCategoriesDisplayedOnHomePage()
	.Select(category =>
	{
	    var catModel = new CategoryModel
	    {
		Id = category.Id,
		Name = _localizationService.GetLocalized(category, x => x.Name),
		Description = _localizationService.GetLocalized(category, x => x.Description),
		MetaKeywords = _localizationService.GetLocalized(category, x => x.MetaKeywords),
		MetaDescription = _localizationService.GetLocalized(category, x => x.MetaDescription),
		MetaTitle = _localizationService.GetLocalized(category, x => x.MetaTitle),
		SeName = _urlRecordService.GetSeName(category),
	    };
	})
	.ToList()

is not efficient.

Those are 6 extra queries for each category. This pattern is used for pretty much everything but mostly for categories, products and manufacturers.

I have used MiniProfiler to profiles some of these queries and I can see we are running hundreds of queries to load a page.
Browsing through the e-commerce is painful as well as most of the entities are not yet cached.

We cannot use LoadAllLocalizedPropertiesOnStartup as we have 200K products ... and growing.

To fix some of these issues I had to comment MetaKeywords, MetaDescription and MetaTitle as we don't used them. This little trick saves me a few seconds.

Shouldn't we load all those properties for an entity instead of each field ? Best would be to load them for multiple entities, maybe using an attribute for the field to identify those localized properties.

@astrumcs
Copy link

@AndreiMaz,

I have implemented the class recommended by the EF Core folks as a work around for the lazy loading issue.

It has been in production for several days and I have not yet seen any issues with it. The performance improvement was significant. A 100 item cart that was taking minutes to load is now consistently loading in less than 2 seconds. Similar improvements were seen in other parts of the site that perform heavy lazy loading or repetitive SQL requests.

Feedback is welcome:

namespace Nop.Data
{
	public class EFLazyLoaderFix : ILazyLoader
	{
		private readonly ILazyLoader _efCoreLazyLoader;
		private readonly ICurrentDbContext _currentContext;

		public EFLazyLoaderFix(ICurrentDbContext currentContext, IDiagnosticsLogger<DbLoggerCategory.Infrastructure> logger)
		{
			_currentContext = currentContext;
			_efCoreLazyLoader = new LazyLoader(currentContext, logger);
		}

		private bool AutoDetectChangesEnabled
		{
			get
			{
				return _currentContext.Context.ChangeTracker.AutoDetectChangesEnabled;
			}
			set
			{
				_currentContext.Context.ChangeTracker.AutoDetectChangesEnabled = value;
			}
		}

		public void Load(object entity, [CallerMemberName] string navigationName = null)
		{
			var originalChangeTrackingState = AutoDetectChangesEnabled;
			try
			{
				AutoDetectChangesEnabled = false;
				_efCoreLazyLoader.Load(entity, navigationName);
			}
			finally
			{
				if (originalChangeTrackingState)
					AutoDetectChangesEnabled = true;
			}
		}

		public async Task LoadAsync(object entity, CancellationToken cancellationToken = default(CancellationToken), [CallerMemberName] string navigationName = null)
		{
			var originalChangeTrackingState = AutoDetectChangesEnabled;
			try
			{
				AutoDetectChangesEnabled = false;
				await _efCoreLazyLoader.LoadAsync(entity, cancellationToken, navigationName);
			}
			finally
			{
				if (originalChangeTrackingState)
					AutoDetectChangesEnabled = true;
			}
		}
	}
}

I also make the following call at the bottom of UseSqlServerWithLazyLoading method in the DbContextOptionsBuilderExtensions class.

optionsBuilder.ReplaceService<ILazyLoader, EFLazyLoaderFix>();

@pantonis
Copy link

pantonis commented Mar 2, 2019

I updated my code rebuilt but I did not see any difference. Single store with 3500 products running on an SSD VPS and sql server 2016 express.

@astrumcs
Copy link

astrumcs commented Mar 7, 2019

@pantonis You added the class above and then modified the DbContextOptionsBuilderExtensions class located in the source tree in this file?

Presentation/Nop.Web.Framework/Infrastructure/Extensions/DbContextOptionsBuilderExtensions.cs

@pantonis
Copy link

pantonis commented Mar 7, 2019

@astrumcs yes I did both. While debugging added breakpoints to NopLazyLoader and they fired.

@astrumcs
Copy link

astrumcs commented Mar 7, 2019

I'll dig back into it and see if there was anything else that changed. What kind of load times are you getting on carts with 30 or 40 separate line items?

@pantonis
Copy link

pantonis commented Mar 7, 2019

It loads in 3 seconds (30 items). When clicking on a category which has about 300 products it takes ages.
Same problem when clicking on a product description takes about 5 seconds.

@astrumcs
Copy link

@pantonis,

Well, the only other I have concerns a check for dependencies between products in the cart. My most recent implementation of nop does not require this. So, here is a link to the code that I ended up skipping because I don't need it. I just issue a return warnings; just above this line.

As I recall, I did this change before I made the change to the lazy loading. Cutting out the required product check helped, but, for me, the lazy loading seemed to be the real improvement.

@rollsch
Copy link

rollsch commented May 2, 2019

What is the state of this. Is the performance worse, the same, or better than the older nopcommerce which used entity framework which was compiled for net461 ?

@astrumcs
Copy link

astrumcs commented May 2, 2019

I would suggest that you look at 4.2 Beta. I have not yet put the beta version onto a test server, but the change long indicates that EF core 2.2 is used and that is supposed to solve the performance issues that I was experiencing.

Your mileage may vary...

@rollsch
Copy link

rollsch commented May 4, 2019

I'm in no rush. My net 461 install of 4.0 is still doing well and I have lots of custom plugins, I don't want to commit to porting all my plugins to netcore unless there will be substantial gains from nop 4.2 with netcore everything.

I was hoping there would be some more conclusive benchmarks or something suggesting if netcore is faster and if not why not.

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

No branches or pull requests

5 participants