4
\$\begingroup\$

The following method retrieves a list of users and the subscriptions for each user, from my database. That said, it works, but it runs pretty dang slow. I'd like to improve the speed on it. Currently, it fetches a mere 90 records, and takes nearly 20 seconds to do so, partly because, for every record of final output, it's grouping the subs per user, and doing a fetch on that user to obtain userId, email, and username (all three as User below).

public IEnumerable<ActiveSubscriptionsSummary> ActiveSubscriptionsByUser(SubscriptionType type)
{
    var subs = new List<ActiveSubscriptionsSummary>();

    var subscriptions = _bss.ActiveSubscriptions(type);
    var subsByUser = subscriptions
        .GroupBy(s => s.UserId,
            s => s,
            (key, g) => new ActiveBuilderSubscriptionsSummary
            {
                User = _cs.ById(key),
                Subscriptions = g.ToList()
            })
        .ToList();

    return subsByUser;
}

_cs.ById above refers to a service call that retrieves the user:

public ICustomer ById(Guid id)
{
    return GetBackend().ById(id);
}

All I use from that method is UserId, UserName, and Email (in the ActiveSubscriptionsByUser method, that is).

_bss above refers to a service call, with this method:

public IEnumerable<Subscription> ActiveSubscriptions(string subscriptionType)
{
    var subs = _sms.CurrentSession.Query<Subscription>()
                    .Where(s => (s.SubscriptionType == subscriptionType) &&
                                (s.EndDate >= DateTime.Today.Date))
                    .ToList();
    return subs;
}

My goal here is to speed this thing up, as I know over time that "90" will be a pretty small number, and we'll potentially be showing 100's, if not 1000's, of subs on the screen. And 20 seconds for 90 equates to a long dang time for 1000+.

Is my GroupBy potentially something that can be better coded to make this run faster? Could I potentially join two queries "users" and "subs" in some better way?

Update This is C#, running against an nHibernate-hooked SQL DB, outputting to Angular.

\$\endgroup\$
2
\$\begingroup\$

Your performance is going to suffer until you can create a "WHERE IN"-style query for users, either by building a string or by combining Expressions.

Or, if you want to keep it simple, just put the active criteria in your query, don't pull it up front. I'd recommend this approach.

Add .Expand("Users") on the active subscriptions query to pull back the related Users. I would avoid getting your ActiveBuilderSubscriptionSummary involved until you get your results back, at which point you can then project the query results to a collection of ActiveBuilderSubscriptionSummary objects using a standard LINQ .Select.

If you are hesitant to remove or change your method because you need to reuse it, or it is comfortably nestled in a repository, I'd refactor to use a stored proc instead. Otherwise, faced with the choice, it is way better to duplicate your query in code than to force an eager load of data up front.

\$\endgroup\$
  • \$\begingroup\$ This is C#, running against an nHibernate-hooked SQL DB, outputting to Angular. I probably ought to put that in the main question so it's clear. \$\endgroup\$ – PKD Oct 15 '15 at 21:15
  • \$\begingroup\$ @PKD updated for your stack. \$\endgroup\$ – moarboilerplate Oct 15 '15 at 21:17
  • \$\begingroup\$ Thanks for the update. Now... if I had a clue how to do what you're saying... \$\endgroup\$ – PKD Oct 15 '15 at 21:27
  • 1
    \$\begingroup\$ @PKD Your ActiveSubscriptions method is the problem because you're loading the data from the DB, then grouping it, then slamming the db with a query for every group. So instead, I'd recommend completely doing away with the ActiveSubscriptions method and making one chained query that pulls active users, then uses .Expand("Subscriptions") to get the related subscriptions. Then call .ToList on that to execute the query and materialize your objects into memory. After that you'll have a list of Users with populated Subscriptions. Use .Select to project that to a list of your custom type. \$\endgroup\$ – moarboilerplate Oct 15 '15 at 21:46
  • 2
    \$\begingroup\$ You've got my upvote, because you're right, but you could possibly make it clearer what's actually happening here. (That the call to ToList() executes the original SQL query and everything else happens in memory after that) and that the solution is to craft an efficient SQL query that puts the grouping work on the database. \$\endgroup\$ – RubberDuck Oct 16 '15 at 0:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.