1

I have just ran into issues with very slow performance of ordering the list. After running a query to database, I get the result and need to order it 4 times before it meets the requirements. The query runs pretty fast and I get all the results almost immediately however sorting the records takes almost 8seconds.

I'm also using pagination therefore each time I select only 50 records per page but I have to reorder the whole list again and again which is a nightmare. Do you guys there have any way to make it run faster ?

var studentMessages = context.Students
    .Where(s => s.SchoolId == SchoolId).ToList();
var sSorted = studentMessages
    .Where(x => x.message == null && x.student.Status != (int)StudentStatusEnum.NotActive)
    .OrderByDescending(x => x.student.UserId)
    .ToList();

sSorted = sSorted
    .Concat(studentMessages
        .Where(x => x.message != null && x.student.Status != (int)StudentStatusEnum.NotActive)
        .OrderBy(x => x.message.NextFollowUpDate)
        .ToList()
    ).ToList();

sSorted = sSorted
    .Concat(studentMessages
        .Where(x => x.message != null && x.student.Status == (int)StudentStatusEnum.NotActive)
        .OrderByDescending(x => x.message.NextFollowUpDate)
        .ToList()
    ).ToList();

sSorted = sSorted
    .Concat(studentMessages
    .Where(x => x.message == null && x.student.Status == (int)StudentStatusEnum.NotActive)
    .OrderByDescending(x => x.user.Id)
    .ToList()
    ).ToList();

var allStudents = (isSelectAll == true ? sSorted  : sSorted .Skip(skipNumber).Take(query.AmountEachPage)).ToList();
Markus
  • 20,838
  • 4
  • 31
  • 55
Edward
  • 37
  • 7
  • Can you show how you assign `studentMessages`? Is the query already executed against the database upon assignment or do you only assign the query (that is executed 4 times whenever you call `ToList`)? – Markus Feb 12 '19 at 08:13
  • pagination does not help a lot if you need the whole list in memory for reordering anyways... – DevilSuichiro Feb 12 '19 at 08:19
  • @DevilSuichiro yes exactly, it's pretty much pointless because I still get all the results over and over again, that's how it was designed in my company but I'm just starting my journey with coding so not sure how to make it better – Edward Feb 12 '19 at 08:23
  • @Markus It's just a simple query var studentMessages = context.Students .Where(s => s.SchoolId == SchoolId) .ToList(); – Edward Feb 12 '19 at 08:27
  • What is the **type** of the `student.UserId` and `message.NextFollowUpDate` properties? – Ivan Stoev Feb 12 '19 at 11:28

2 Answers2

1

The performance problems of the Code are most likely a result of lazy loading. By using the student and message property (and in the case of the fourth query also the user property), the database is queried for each row again. The more rows studentMessage contains, the slower the code will perform. This is a so called "n+1 SELECTs" problem. For details, see this link.

If you want to solve the problem quickly, you need to assert that the relevant sub-entities are also loaded with the first request. To do this, you need to change the following line and include all relevant entities:

var studentMessages = context.Students
  .Where(s => s.SchoolId == SchoolId)
  .ToList();    

should be changed so that the entities message, user and student are also included:

var studentMessages = context.Students
  .Include(x => x.message)
  .Include(x => x.student)
  .Include(x => x.user)
  .Where(s => s.SchoolId == SchoolId)
  .ToList();    

This way, the data are loaded with one request to the database and not later on.

Markus
  • 20,838
  • 4
  • 31
  • 55
1

I think the cause of your problems is because you get subsets or your sequence and order this subset. You do this several times, and you decide to make Lists of all intermediate results.

Let's first see how you want to order your students.

So you have a schoolId and a sequence of Students. Every Student has properties SchoolId, Message, and Status

You take all Students from the school with schoolId, and for some reason you decide to call these students studentMessages.

Then you want to order these Students (studentmessages) in the following order:

  • First all Students with null message and Status unequal to notActive, ordered by descending UserId
  • Then all Students with non-null message and Status unequal to notActive, ordered by Message.NextFollowUpdate
  • Then all Students with non-null message and status equal to notActive, ordered by Message.NextFollowUpdate
  • Finally all Students with null message and status equal to notActive, ordered by descending User.Id (sure you didn't mean UserId? I think that would be the same)

In a table:

group | Message |  Status      | Order by
  1   | == null | != notActive | descending UserId
  2   | != null | != notActive | ascending  message.NextFollowUpdate
  3   | != null | == notActive | descending message.NextFollowUpdate
  4   | == null | == notActive | ascending  UserId

One of the methods would be to let your database management system do this (AsQueryable). The ordering algorithm seems fairly complicated. I'm not sure whether the DBMS can do this more efficient than your process.

Another method would be to fetch only the Students you actually need and let your process do the ordering (AsEnumerable). Provide a class that implements IComparer<Student> to decide about the order.

int schoolId = ...
IComparer<Student> mySpecialStudentComparer = ...
var orderedStudents = dbContext.Students
    .Where(student => student.SchoolId == schoolId)
    .AsEnumerable()         // move the selected data to local process

    // now that the data is local, we can use our local Student Comparer
    .OrderBy(mySpecialStudentComparer);

If your Student has a lot of properties that you won't use after you fetched the data, consider creating a local class that contains only the properties you need and limit the selected data to this local class, for example FetchedStudent

    .Select(student => new FetchedStudent
    {
        // select only the properties you actually plan to use,

        // for the sorting we need at least the following:
        Message = student.Message,
        Status = student.Status
        UserId = student.UserId,

        // Select the other Student properties you plan to use, for example:
        Id = student.Id,
        Name = student.Name, 
        ...
    }

Of course in that case, your comparer needs to implement IComparer<FetchedStudent>.

So let's create a StudentComparer that will sort the Students according to your requirements!

class StudentComparer : IComparer<FetchedStudent>
{
    private readonly IComparer<int> UserIdComparer = Comparer<int>.Default;
    private readonly IComparer<DateTime> nextFollowUpdateComparer =
                     Comparer<DateTime>.Default;

    public int CompareTo(FetchedStudent x, FetchedStudent y)
    {
        // TODO: decide what to do with null students: exception?
        // or return as smallest or largest

        // Case 1: check if x is in sorting group 1
        if (x.Message == null && x.Status == notActive)
        {
            // x is in sorting group 1
            if (y.Message == null && y.Status == notActive)
            {
                // x and y are in sorting group 1.
                // order by descending UserId
                return -UserIdComparer.CompareTo(x.UserId, y.UserId);
                // the minus sign is because of the descending
            }
            else
            {   // x is in group 1, y in group 2 / 3 / 4: x comes first
                return -1;
            }
        }

        // case 2: check if X is in sorting group 2
        else if (x.Message != null && x.Status != notActive)
        {   // x is in sorting group 2
            if (y.Message == null && y.Status != notActive)
            {   // x is in group 2; y is in group 1: x is larger than y
                return +1;
            }
            else if (y.Message == null && y.Status != notActive)
            {   // x and y both in group 2: order by descending nextFollowUpDate
                // minus sign is because descending
                return -nextFollowUpdateComparer.CompareTo(
                       x.Message.NextFollowUpdate,
                       y.Message.NextFollowUpdate);
            }
            else
            {   // x in group 2, y in 3 or 4: x comes first
                return -1;
            }
        }

        // case 3: check if X in sorting group 3
        else if (x.Message == null && x.Status != notActive)
        {
           ... etc, you'll know the drill by know
    }
}    

Possible improvement

You see that the comparer constantly compares whether x.Message equal null and whether x.Status equals notActive, to detect to which sorting group x and y belong.

Consider creating a function that only once calculates to which sorting group a Student belongs and remember the sorting group:

.Select(student => new FetchedStudent
{
    SortingGroup = student.ToSortingGroup(),

    ... // other properties you need
}

public int CompareTo(FetchedStudent x, FetchedStudent y)
{
    switch (x.SortingGroup)
    {
        case 1:
           switch y.SortingGroup:
           {
               case 1: // x and y both in sorting group 1
                  return -UserIdComparer.CompareTo(x.UserId, y.UserId);

               default: // x in sorting group 1, y in 2 / 3 / 4: x smaller
                  return -1;
           }
        case 2:
           switch y.SortingGroup:
           {
               case 1: // x in sorting group 2; y in sorting group 1: x larger
                  return +1;
               case 2: // x and y both in sorting group 2
                  return -nextFollowUpdateComparer.CompareTo(
                       x.Message.NextFollowUpdate,
                       y.Message.NextFollowUpdate);
           }    

etc. This way the comparison with Message and Status is only done once

Harald Coppoolse
  • 28,834
  • 7
  • 67
  • 116
  • That's an amazing and very thorough explanation, thank you so much for your time I really appreciate it ! – Edward Feb 13 '19 at 01:53