Nopcommerce: Slow performance when many products are loaded on the same page

Created on 12 Nov 2018  路  12Comments  路  Source: nopSolutions/nopCommerce

I have a production site where users have added up to 1400 wish list items to their account. Site runs ok until a user with a high number of wish list items requests a view of their wish list. At that point the server starts bogging down. More /wishlist view requests will bring the server to its knees. Problem is exacerbated by a VM server with limited resources. To fix requires recycling the App Pool.

Have a site with several thousand SKUs. Add 1200 entries to wish list. Request the /wishlist view or any other page when thousands of products should be loaded on one page. Users will usually hit the browser stop button and retry 3 or 4 times. The requests will queue up.

Also see related work item https://github.com/nopSolutions/nopCommerce/issues/3363 and https://github.com/nopSolutions/nopCommerce/issues/3402 (related to locales when multiple languages enabled)

refactoring / source code

Most helpful comment

My basic change is to insert this line above the problem line listed above:

// The code for passedProductRequiredQuantity is not necessary if there are not any 
// other products in the cart that have RequireOtherProducts.  So, do a quick check 
// for those products before doing the way more expensive work that passedProductRequiredQuantity does
var cartProductIds = cart.Select(r => r.ProductId);
var products = _productRepository.TableNoTracking.Where(r => cartProductIds.Contains(r.Id));
if (!products.Any(r => r.RequireOtherProducts))
    return warnings;

Under my testing setup, this resulted in a 10x improvement in page load for a 20 item wishlist.

Additionally, I have implemented the paged wishlist so that there are only 20 or so items being retrieved and displayed at once.

All 12 comments

@astrumcs I had opened an issue for the same problems.
There's a fix to mitigate problems with RAM usage. Just run the app pool at 32bit and implement a couple of changes to the config files.
Queries will be slow still.
There a few issues with localizations. Every time and entity or a set of entity is loaded the repository layer loads a localized property for each field.
For categories, manufactures and products that means running at least 6 queries for each entity.
At some point those info are cached but still.
I guess it's crazy but there's not much we can do about it.

Bunch of issues with performance on this platform. Far as i know implementing elastic search for catalog indexing of view models/languages minus any pricing logic has huge gains in performance. The redis cache implementation is broke since they treat it as if it were in-memory instead of remote (100 of hits to the remote cache per page-load) and no async. Every app pool refresh or cache expire needs to reload the whole thing again, 100's of queries per page almost. it's great example of why lazy loading is dangerous and setting things up to load items it needs up front in one query....

@clearbucketLabs yep. This code base is poorly written and lots of implementation don't consider performance and resources management at all.
I have had exposure to Azure and on that platform resources (queries, memory and CPU) are really expensive.
I am working on a fork now and trying to optimize the product a bit. It's not easy.
Do you have any code to share for the Elastic Search integration ?

@Leftyx Thanks for the reference back to your issue entry. Definitely gives some food for thought on solving my problem. I'll be focusing on this over the next few days and will pass on any epiphanies I run into.

I'm finishing up my changes to use a paged wishlist instead of the full list being returned.

After quite a bit of looking around and analysis, one of the big performance killers is this statement:

//whether other cart items require the passed product

var passedProductRequiredQuantity = cart
  .Where(item => item.Product.RequireOtherProducts && 
  _productService.ParseRequiredProductIds(item.Product).Contains(product.Id))
  .Sum(item => item.Quantity * requiredProductQuantity);

This line is run in _ShopperCartService.cs_ in the _GetRequiredProductWarnings_ method. With a large wishlist (or shopping cart for that matter). This line will just kill performance. On top of that, the use case is probably one that is not used very often. So it ends up being a check that is probably not needed on many installations.

My fix is to check if there are any products in the cart that have the _RequireOtherProducts_ bit set. If there are not any, the Linq query is skipped. Huge gain.

In my mind, it seems that the real fix is to break out this functionality into a many to many database table that will contain and define the relationship between products for required products. This would change the check to a fairly simple (and fast) check of product.RequiredProducts.Any() against the database. The check in the reverse direction would be just as quick and simple: product.RequiredBy.Any() or something similar.

My basic change is to insert this line above the problem line listed above:

// The code for passedProductRequiredQuantity is not necessary if there are not any 
// other products in the cart that have RequireOtherProducts.  So, do a quick check 
// for those products before doing the way more expensive work that passedProductRequiredQuantity does
var cartProductIds = cart.Select(r => r.ProductId);
var products = _productRepository.TableNoTracking.Where(r => cartProductIds.Contains(r.Id));
if (!products.Any(r => r.RequireOtherProducts))
    return warnings;

Under my testing setup, this resulted in a 10x improvement in page load for a 20 item wishlist.

Additionally, I have implemented the paged wishlist so that there are only 20 or so items being retrieved and displayed at once.

The lazy loading fix described in #3419 made huge gains for this problem as well.

I ended up leaving the check that I outline above in the production code because it really does save unnecessary processing in many cases.

Thanks. Right now I can only recommend you to use "ShoppingCartSettings.MaximumShoppingCartItems" and "ShoppingCartSettings.MaximumWishlistItems" settings. By default their values are set to 1000. But we understand that in real life 99% of stores don't need it. Paging support is also not the best way to go as it's not really user-frienly

P.S. The issue described in https://github.com/nopSolutions/nopCommerce/issues/3402 can be resolved with NoTracking. But this approach for wishlist won't work as it'll make source code really not developer-friendly

Please note that the suggestion provided by astrumcs on 17 Nov 2018 won't help because it just loads a product for each wishlist item. But even if you rename it they'll be loaded later anyway (for example, to display a product name).

Here are several commits that can help you:

  1. https://github.com/nopSolutions/nopCommerce/commit/3f5bdf5e5aae952ca193f20bff1b46a413108f96
  1. https://github.com/nopSolutions/nopCommerce/commit/3451cb8fd985ce798c871311248ca7f5d1911e26

  2. https://github.com/nopSolutions/nopCommerce/commit/4ffbd11858873f44350786656144fb9ae9f0e66f

@astrumcs Is it possible for you to test the latest "develop" branch?

@AndreiMaz, I will try and test the latest develop branch. I'm not sure if I have time, too many to do items. If I do, I will let you know what I discover.

optimized in the upcoming 4.30 after moving away from EF and caching between HTTP requests

Was this page helpful?
0 / 5 - 0 ratings

Related issues

DmitriyKulagin picture DmitriyKulagin  路  5Comments

mrnams picture mrnams  路  3Comments

AndreiMaz picture AndreiMaz  路  3Comments

tigran-avdalyan picture tigran-avdalyan  路  7Comments

AndreiMaz picture AndreiMaz  路  6Comments