Very slow query, how can I improve it?

Question

I have the following abstract class:

public abstract class Notification
{
    public Notification()
    {
        this.receivedDate = DateTime.Now.ToUniversalTime();
    }

    public int Id { get; set; }

    public DateTime receivedDate { get; set; }

    public bool unread { get; set; }

    public virtual ApplicationUser recipient { get; set; }
}

Several classes inherit from it, for instance ProfileViewNotification and NewMessageNotification:

public class ProfileViewNotification: Notification
{
    public virtual ApplicationUser Viewer { get; set; }
}

public class NewMessageNotification: Notification
{
    public virtual Message Message { get; set; }
}

I have the following method to query my DB for all the Notification with a given ApplicationUser as a recipient :

public static List<NotificationApiViewModel> GetNotificationsForUser(string idOfUser)
    {
        List<NotificationApiViewModel> resultAsApiViewModel = new List<NotificationApiViewModel>();

        List<ProfileViewNotification> ofProfileViewNotificationType = null;
        List<NewMessageNotification> ofNewMessageNotificationType = null;

        try
        {
            using (var context = new ApplicationDbContext())
            {
                var query = context.Notifications.Where(c => c.recipient.Id == idOfUser);

                ofNewMessageNotificationType = query.OfType<NewMessageNotification>().Any() ? 
                    query.OfType<NewMessageNotification>()
                    .Include(n => n.recipient)
                    .Include(n => n.recipient.MyProfile)
                    .Include(n => n.recipient.MyProfile.ProfileImages)
                    .Include(n => n.Message)
                    .Include(n => n.Message.Author)
                    .Include(n => n.Message.Author.MyProfile)
                    .Include(n => n.Message.Author.MyProfile.ProfileImages)
                    .Include(n => n.Message.Recipient)
                    .Include(n => n.Message.Recipient.MyProfile)
                    .Include(n => n.Message.Recipient.MyProfile.ProfileImages)
                    .ToList() 
                    : null;

                ofProfileViewNotificationType = query.OfType<ProfileViewNotification>().Any() ? 
                    query.OfType<ProfileViewNotification>()
                    .Include(n => n.recipient)
                    .Include(n => n.recipient.MyProfile)
                    .Include(n => n.recipient.MyProfile.ProfileImages)
                    .Include(n => n.Viewer)
                    .Include(n => n.Viewer.MyProfile)
                    .Include(n => n.Viewer.MyProfile.ProfileImages)
                    .ToList() 
                    : null;

                }

        }
        catch (Exception ex)
        {
            //Log issue
        }

        if (ofNewMessageNotificationType != null)
        {
            foreach (var n in ofNewMessageNotificationType)
            {
                resultAsApiViewModel.Add(NotificationApiViewModel.ConvertToApiViewModel(n));
            }
        }

        if (ofProfileViewNotificationType != null)
        {
            foreach (var n in ofProfileViewNotificationType)
            {
                resultAsApiViewModel.Add(NotificationApiViewModel.ConvertToApiViewModel(n));
            }
        }

        return resultAsApiViewModel;
    }

Important to note, none of my ConvertToApiViewModel methods query the DB, that's why I have all these Include in the original query. Also the above only includes 2 types of notifications for the sake of brevity but I have a dozen in total.

My problem is that my method is extremely slow. For a user who has a mere 20 notifications it takes over a minute to complete!

Anyone can tell me what I am doing wrong?

I guess you should take a look at the generated sql queries and profile them to see if you're missing some indexes. — Kinetic, Jul 20 '16 at 03:26
@KiNeTiC thanks for your comment. Could you guide me on how to do that? I don't know how to look at the generated sql queries and I am not sure what you mean by "missing some indexes" — aBertrand, Jul 20 '16 at 03:40
For complex queries like this (lots of includes), I suggest you use LINQ instead because you can select the required fields easily... What you are doing here is essentially getting everything from database, which is far from optimal. — Rosdi Kasim, Jul 20 '16 at 03:42
The problem is the amount of nested includes, it generates too many `joins` in the sql query. I think there is no EF workaround, so you should use sql queries. Take a look at http://stackoverflow.com/questions/2086894/optimizing-multiple-joins — Taher Rahgooy, Jul 20 '16 at 03:44
How to get the generated queries from EF6 : http://stackoverflow.com/questions/1412863/how-do-i-view-the-sql-generated-by-the-entity-framework — Kinetic, Jul 20 '16 at 03:44
As far as SQL optimization and queries go, it really depends on what kind of database you're using. — Kinetic, Jul 20 '16 at 03:45
As I see, you get a lot things from database, you even Include images for all notifications as I see. Can you get brief notification info for user and load needed data after the user selects which notification it wants to see for example? — Adil Mammadov, Jul 20 '16 at 06:23

Shai Aharoni · Answer 1 · 2016-07-20T03:40:37.217

You are making a call to the DB for each of your dozen queries so instead what your could do is combine them to a single query like this:

  try
            {
                using (var context = new ApplicationDbContext())
                {
                    //Here you execute the single query
                    var query = context.Notifications.Where(c => c.recipient.Id == idOfUser)
 .Include(n => n.recipient)
                        .Include(n => n.recipient.MyProfile)
                        .Include(n => n.recipient.MyProfile.ProfileImages)
                        .Include(n => n.Message)
                        .Include(n => n.Message.Author)
                        .Include(n => n.Message.Author.MyProfile)
                        .Include(n => n.Message.Author.MyProfile.ProfileImages)
                        .Include(n => n.Message.Recipient)
                        .Include(n => n.Message.Recipient.MyProfile)
                        .Include(n => n.Message.Recipient.MyProfile.ProfileImages)
.Include(n => n.Viewer)
                        .Include(n => n.Viewer.MyProfile)
                        .Include(n => n.Viewer.MyProfile.ProfileImages)
.ToList(); 

                    ofNewMessageNotificationType = query.OfType<NewMessageNotification>().Any() ? 
                        query.OfType<NewMessageNotification>(): null;

                    ofProfileViewNotificationType = query.OfType<ProfileViewNotification>().Any() ? 
                        query.OfType<ProfileViewNotification>() : null;

                    }

            }
            catch (Exception ex)
            {
                //Log issue
            }

            if (ofNewMessageNotificationType != null)
            {
                foreach (var n in ofNewMessageNotificationType)
                {
                    resultAsApiViewModel.Add(NotificationApiViewModel.ConvertToApiViewModel(n));
                }
            }

            if (ofProfileViewNotificationType != null)
            {
                foreach (var n in ofProfileViewNotificationType)
                {
                    resultAsApiViewModel.Add(NotificationApiViewModel.ConvertToApiViewModel(n));
                }
            }

            return resultAsApiViewModel;
        }

Hope this helps...

Sorry I don't think you can do that, if you want to use `Include` you need to query again, one cannot do `Include` after having done `ToList()` as far as I am aware because `Include` is performed to query virtual entities that the original query hadn't fetched in the first place. — aBertrand, Jul 20 '16 at 03:38
You are right, but what you can do is to make all your includes in the single query. I updated my answer — Shai Aharoni, Jul 20 '16 at 03:42
Sorry doesn't work either because the original query queries `Notification` types and it therefore doesn't know about `Message` or `Viewer`. — aBertrand, Jul 20 '16 at 03:46
This answer doesn't make much sense to me. I can't understand how this would make the queries faster. — Kinetic, Jul 20 '16 at 03:47
@aBertrand doesn't your Notification DB table has a MessageId & ViewerId columns that associate the notification with a message and a viewer? — Shai Aharoni, Jul 20 '16 at 03:52
@ShaiAharoni i think he means that he can't use Include anymore since the base type doesn't have the types he needs to join on. I found something similar here http://stackoverflow.com/questions/6263288/eager-loading-property-of-derived-class-using-include and still an open request https://github.com/aspnet/EntityFramework/issues/3910 — hdz, Jul 20 '16 at 04:38

score 0 · Accepted Answer · answered Jul 24 '16 at 02:08

Thanks everyone for your comments and replies. For the record the way I made the query faster is by using query.Select(n => new DTO{}) and Data Transfer Objects (DTOs) instead of my multiple Include. Just with this I improved the performance by an order of magnitude. I also made the queries asynchronous which further improved performance.

Very slow query, how can I improve it?

2 Answers2