Nop 3.8 - 3.3 GigaByte Memory usage!

a.m. wrote:

LOL - Did you just say this?
These two methods cannot be the case because they do not load all records into memory. They load only one record and then use "TotalCount" property. Please see "pageSize" property (we pass 1)

Did you seriously just say that?

I debugged your search order query and wrote it the end of this message for reference.
It loads everything into memory. Of course it does - there is no magic - what the heck are you thinking?
Your page size property is only about how many record you want to return to the client. It has nothing to do with how many record are returned from the DB.

The query is then put it into a paged list - but the paged list is a simple glorified IList that you use to page the data before sending it to the client. It does nothing to reduce how much data is retrieved from the DB.

As soon as your pagedlist called source.count() - everything is loaded (Basic IEnumerable behavior)

Andrei - this is your code - I don't want to tell you how it works.

Jesus - your response is freaking me out.
If you guys do not understand this - then how can we hope that you will fix it?

I will make one more attempt at explaining the issue again -
All record are loaded in the DB.
All 125,000 orders and all 75,000 customers
Then you do a count on it! And all you need is the result of the count - so it is a mega waist of resources as you can imagine.
I re enabled your query to further debug it and show you sql statement below.
Guess what - my memory consumption is back 3,7 Giga byte right now :)
Oh - and the query takes a very long time to return.

When I apply the code chnages I did yesterday. It loads only 1 integer from the DB and the code returns very fast of course.

I suggested you guys test your code before becuase I think it will help the team understand much better how the code actually works.

I just spent 2 more hours to show you that what you just said in your last message was wrong - since we are both very busy please lets stop arguing.
If you do not want to believe what I am showing you. Then I simply cannot help you.

Here is query your code runs - it returns everything from that query and later on your code will do a count on the IEnumerable:
SELECT       [Extent1].[Id] AS [Id],      
[Extent1].[OrderGuid] AS [OrderGuid],      
[Extent1].[StoreId] AS [StoreId],      
[Extent1].[CustomerId] AS [CustomerId],      
[Extent1].[BillingAddressId] AS [BillingAddressId],      
[Extent1].[ShippingAddressId] AS [ShippingAddressId],      
[Extent1].[PickupAddressId] AS [PickupAddressId],      
[Extent1].[PickUpInStore] AS [PickUpInStore],      
[Extent1].[OrderStatusId] AS [OrderStatusId],      
[Extent1].[ShippingStatusId] AS [ShippingStatusId],      
[Extent1].[PaymentStatusId] AS [PaymentStatusId],      
[Extent1].[PaymentMethodSystemName] AS [PaymentMethodSystemName],      
[Extent1].[CustomerCurrencyCode] AS [CustomerCurrencyCode],      
[Extent1].[CurrencyRate] AS [CurrencyRate],      
[Extent1].[CustomerTaxDisplayTypeId] AS [CustomerTaxDisplayTypeId],      
[Extent1].[VatNumber] AS [VatNumber],      
[Extent1].[OrderSubtotalInclTax] AS [OrderSubtotalInclTax],      
[Extent1].[OrderSubtotalExclTax] AS [OrderSubtotalExclTax],      
[Extent1].[OrderSubTotalDiscountInclTax] AS [OrderSubTotalDiscountInclTax],      
[Extent1].[OrderSubTotalDiscountExclTax] AS [OrderSubTotalDiscountExclTax],      
[Extent1].[OrderShippingInclTax] AS [OrderShippingInclTax],      
[Extent1].[OrderShippingExclTax] AS [OrderShippingExclTax],      
[Extent1].[PaymentMethodAdditionalFeeInclTax] AS [PaymentMethodAdditionalFeeInclTax],      
[Extent1].[PaymentMethodAdditionalFeeExclTax] AS [PaymentMethodAdditionalFeeExclTax],      
[Extent1].[TaxRates] AS [TaxRates],      
[Extent1].[OrderTax] AS [OrderTax],      
[Extent1].[OrderDiscount] AS [OrderDiscount],      
[Extent1].[OrderTotal] AS [OrderTotal],      
[Extent1].[RefundedAmount] AS [RefundedAmount],      
[Extent1].[RewardPointsWereAdded] AS [RewardPointsWereAdded],      
[Extent1].[CheckoutAttributeDescription] AS [CheckoutAttributeDescription],      
[Extent1].[CheckoutAttributesXml] AS [CheckoutAttributesXml],      
[Extent1].[CustomerLanguageId] AS [CustomerLanguageId],      
[Extent1].[AffiliateId] AS [AffiliateId],      
[Extent1].[CustomerIp] AS [CustomerIp],      
[Extent1].[AllowStoringCreditCardNumber] AS [AllowStoringCreditCardNumber],      
[Extent1].[CardType] AS [CardType],      
[Extent1].[CardName] AS [CardName],      
[Extent1].[CardNumber] AS [CardNumber],      
[Extent1].[MaskedCreditCardNumber] AS [MaskedCreditCardNumber],      
[Extent1].[CardCvv2] AS [CardCvv2],      
[Extent1].[CardExpirationMonth] AS [CardExpirationMonth],      
[Extent1].[CardExpirationYear] AS [CardExpirationYear],      
[Extent1].[AuthorizationTransactionId] AS [AuthorizationTransactionId],      
[Extent1].[AuthorizationTransactionCode] AS [AuthorizationTransactionCode],      
[Extent1].[AuthorizationTransactionResult] AS [AuthorizationTransactionResult],      
[Extent1].[CaptureTransactionId] AS [CaptureTransactionId],      
[Extent1].[CaptureTransactionResult] AS [CaptureTransactionResult],      
[Extent1].[SubscriptionTransactionId] AS [SubscriptionTransactionId],      
[Extent1].[PaidDateUtc] AS [PaidDateUtc],      
[Extent1].[ShippingMethod] AS [ShippingMethod],      
[Extent1].[ShippingRateComputationMethodSystemName] AS [ShippingRateComputationMethodSystemName],      
[Extent1].[CustomValuesXml] AS [CustomValuesXml],      
[Extent1].[Deleted] AS [Deleted],      
[Extent1].[CreatedOnUtc] AS [CreatedOnUtc],      
[Extent2].[Id] AS [Id1]      
FROM  [dbo].[Order] AS [Extent1]      LEFT OUTER JOIN [dbo].[RewardPointsHistory] AS [Extent2] ON ([Extent2].[UsedWithOrder_Id] IS NOT NULL) AND ([Extent1].[Id] = [Extent2].[UsedWithOrder_Id])      
WHERE [Extent1].[Deleted] <> 1      
ORDER BY [Extent1].[CreatedOnUtc] DESC

The fix is to simply run a query from the OrderService:
        public virtual int GetOrderCountAll(int storeId = 0)
        {
            string sql = "SELECT Count(*) FROM [dbo].[Order]";
            if (storeId != 0)
            {
                sql += " WHERE [StoreId] = @StoreId";
            }
            var nbOfOrders = _dbContext.SqlQuery<int>(sql, storeId);
            return nbOfOrders.FirstOrDefault();
        }
Of course I added it to the interface and made the same fix for the same issue with getting all the customers.


Please check your code - see what it really does -
Also - please - do not argue with people - this is not complicated.
It is very simple code - very easy to understand and debug.
There really is no place to argue - it is what it is -
If you believe the code does something else than what it does - simply debug it - test it with real data like I suggested before - if you follow the code and do some performance analysis and you will know the truth right away. There is absolutely nothing to argue about.

IT IS FIXED NOW! nopCommerce is flying!!!

Oh my god it is much faster now.

Before this fix - with only 125,000 orders and 70,000+ customer I could bring nopCommerce to use 8 Gigabyte of memory simply by opening through a few Adminstration pages. And these admistration pages would load in around 90 seconds.
Now with this fix - I can open pages in a few seconds. And nopCommerce only uses 600MB - it is an amazing difference.

It is going to save me a lot of money in server deployment - FANTASTIC.
To be honest - I dont think I could have deployed the app before - but now I have a lot more confidence I can.

Andrei - you owe me a big one on this one. A very big one.
I have never worked for free in my life - and to be honest my consulting fees are $250 per hour.
I had to give up my entire weekend to fix the performance issues of nopCommerce 3.8.
I think I just spent at least 20 hours this weekend just fixing all the performance issues I could find.

Don't get me wrong - I am very glad I did. What an amazing difference.
But the bugs I found are so massive - anyone should have been able to see it.

Anyhow - nopCommerce is flying now (compared to before)
I don't think I have ever seen such dramatic improvement for such simple coding mistake.

Life is good again. I will sleep very well tonight.

Frederic

FredBell wrote:
Can you share your new code with us. So that we can check that too and check the difference. :)

Hi Frederic,

You're absolutely right. We haven't tested it enough. This critical performance bug was introduced in version 3.80 in this commit. "PagedList" class was refactored wrong way (IQueryable was replaced with IEnumarable). It'll be fixed very soon. And we'll release a new version with bug fixes soon (with this one class replaced)

FredBell wrote:
Absolutely. How about a free ticket to nopCommerce Days 2016? I would like to invite you to this conference and personally shake your hand. Please PM me if you have an opportunity to join us

a.m. wrote:
Please see this commit. Or you can re-download the update packages here if you don't want to apply the fix manually

a.m. wrote:

Will you release a new official build of nop 3.80 ? Or for later installations, we need to build it from Github, containing this fix ?


Thanks!

Yes, a new build of 3.80

In the class that I rewrote I put some comments that you might want to read.

There are around 60 reference to PagedList.
And some of them are passing a list. In which case you can keep old code legacy compatibility if you wish.

Also make sure that all all o PagedList from Iqueriable are calling OrderBy before by passed in. Otherwsie it will break the code.

Could you create a 3.81 version? Just to make sure people download the right version - it will be a lot easier for everyone to track.

I also did some refactoring on other part of the code base. But this was the big one.

You can find some of the comments here:



using System;
using System.Collections.Generic;
using System.Linq;

namespace Nop.Core
{
    /// <summary>
    /// Paged list
    ///
    /// VSCODE - very important - change all the signature to take <see cref="IQueryable{T}"/> - otherwise with an <see cref="IEnumerable{T}"/>:
    /// none of the nice features of EntityFramework like Counting and Paging worked and EntityFramework returned all the data
    /// (despite what Andrei would like me to believe :) )
    ///
    /// Before this fix - with only 125,000 orders and 70,000+ customer I could bring nopCommerce to use 8 Gigabyte of memory simply by opening through a few Adminstration pages. And these admistration pages would load in around 90 seconds.
    /// Now with this fix - I can open pages in a few seconds. And nopCommerce only uses 800MB - it is an amazing difference.
    ///
    /// To benefit from the major speed improvement - you must call the constructor taking an <see cref="IQueryable{T}"/>
    /// and also you msut make sure that the <see cref="IQueryable{T}"/> is already sorted before using the <see cref="PagedList{T}"/>.
    /// Otherwise, you will get a server side error telling to sort your query.
    ///
    /// Now everything is beautiful again :)
    /// Courtesy of Frederic
    /// </summary>
    /// <typeparam name="T">T</typeparam>
    [Serializable]
    public class PagedList<T> : List<T>, IPagedList<T>
    {
        /// <summary>
        /// Ctor (paging in performed inside)
        /// </summary>
        /// <param name="source">source</param>
        /// <param name="pageIndex">Page index</param>
        /// <param name="pageSize">Page size</param>
        public PagedList(IQueryable<T> source, int pageIndex, int pageSize)
        {
            Init(source, pageIndex, pageSize);
        }
        /// <summary>
        /// VSCODE - Here for legacy reason - there is a lot of code accross nopCommerce that is passing <see cref="IList{T}"/> or <see cref="IEnumerable{T}"/>.
        /// In some cases it was OK because the data was small or not at all related to Entity Framework (not entities).
        /// For the vast majority of cases - where the callers passe Entities - you must call the other constructors passing an <see cref="IQueryable{T}"/> and also make sure that <see cref="IQueryable{T}"/> is already sorted before using this <see cref="PagedList{T}"/>.
        /// </summary>
        /// <param name="source"></param>
        /// <param name="pageIndex"></param>
        /// <param name="pageSize"></param>
        [Obsolete("This constructor is only here to support legacy code - you really should only pass an Iquerable to a PagedList - otherwise EntityFramework will not be able to do server side paging. Courtesy of Frederic")]
        public PagedList(IEnumerable<T> source, int pageIndex, int pageSize, int? totalCount = null)
        {
            InitLegacy(source, pageIndex, pageSize, totalCount);
        }
        /// <summary>
        /// Ctor (already paged source is passed)
        /// </summary>
        /// <param name="source">source</param>
        /// <param name="pageIndex">Page index</param>
        /// <param name="pageSize">Page size</param>
        /// <param name="totalCount">Total count</param>
        public PagedList(IQueryable<T> source, int pageIndex, int pageSize, int? totalCount = null)
        {
            Init(source, pageIndex, pageSize, totalCount);
        }

        /// <summary>
        /// Initialize
        /// </summary>
        /// <param name="source">source</param>
        /// <param name="pageIndex">Page index</param>
        /// <param name="pageSize">Page size</param>
        /// <param name="totalCount">Total count</param>
        [Obsolete("This method is only here to support legacy code - you really should only pass an Iquerable to a PagedList - otherwise EntityFramework will not be able to do server side paging. Courtesy of Frederic")]
        private void InitLegacy(IEnumerable<T> source, int pageIndex, int pageSize, int? totalCount = null)
        {
            if (source == null)
                throw new ArgumentNullException("source");
            if (pageSize <= 0)
                throw new ArgumentException("pageSize must be greater than zero");

            TotalCount = totalCount ?? source.Count();
            TotalPages = TotalCount / pageSize;
            if (TotalCount % pageSize > 0)
                TotalPages++;

            PageSize = pageSize;
            PageIndex = pageIndex;

            source = totalCount == null ? source.Skip(pageIndex * pageSize).Take(pageSize) : source;
            AddRange(source);
        }

        /// <summary>
        /// Initialize
        /// </summary>
        /// <param name="source">source</param>
        /// <param name="pageIndex">Page index</param>
        /// <param name="pageSize">Page size</param>
        /// <param name="totalCount">Total count</param>
        private void Init(IQueryable<T> source, int pageIndex, int pageSize, int? totalCount = null)
        {
            if (source == null)
                throw new ArgumentNullException("source");
            if (pageSize <= 0)
                throw new ArgumentException("pageSize must be greater than zero");

            TotalCount = totalCount ?? source.Count();
            TotalPages = TotalCount / pageSize;
            if (TotalCount % pageSize > 0)
                TotalPages++;

            PageSize = pageSize;
            PageIndex = pageIndex;

            source = totalCount == null ? source.Skip<T>(pageIndex * pageSize).Take<T>(pageSize) : source;
            AddRange(source);
        }

        public int PageIndex { get; private set; }
        public int PageSize { get; private set; }
        public int TotalCount { get; private set; }
        public int TotalPages { get; private set; }

        public bool HasPreviousPage
        {
            get { return (PageIndex > 0); }
        }
        public bool HasNextPage
        {
            get { return (PageIndex + 1 < TotalPages); }
        }
    }
}

On a slightly different topic.

I am writing some new functionality on top of nopCommerce.

Some of it I might want to give it back to the community - simply because I dont want to have to keep track of changes and it requires making some changes to your code base - small changes

Is it possible for me to be able to check in code to your dev branch?

And I have more perf improvement change I could check in too.

But I am more thinking in terms of smaller changes required to make nopCommerce better.

Let me know if I should PM you to discuss this.

As for the invite - I truly appreciate it. But I live in Colorado so it will be difficult for me to make it to Amsterdam. Great city though - I used to live in Amsterdam for 12 years.
So maybe I will go - I'd love to meet you guys - who knows if I will have the opportunity... I run an ecommerce company for the last few years - so you know how it is. When you are the boss you don't have much time to do anything else. It should not be that way I guess, but it certainly is :)


Thank you very much for your last message. It was very nice.

FredBell wrote:
Yes, any contribution is welcome. Please create a pull request as described here

FredBell wrote:
Sure. Please PM me if you decide to go...