1

I have a controller as below, and it takes too long load the data. I am using contains and tolist() methods. And i have heard about low performance of toList() method.

How can i change this approach with better coding for performance.

public List<decimal> GetOrgSolution()
{
    //Need to use USER id. but we have EMPNO in session. 
    var Users = db.CRM_USERS.Where(c => c.ID == SessionCurrentUser.ID || RelOrgPerson.Contains(c.EMPNO.Value)).Select(c => c.ID);

    //Get the organization list regarding to HR organization
    var OrgList = db.CRM_SOLUTION_ORG.Where(c => c.USER_ID == SessionCurrentUser.ID || Users.Contains(c.USER_ID.Value)).Select(c => c.ID).ToList();

    //Get related solutions ID with the OrgList
    List<decimal> SolutionList = db.CRM_SOLUTION_OWNER.Where(p => OrgList.Contains(p.SOLUTION_ORG_ID.Value)).Select(c => (decimal)c.SOLUTION_ID).Distinct().ToList();

    return SolutionList;
}
umki
  • 769
  • 13
  • 31
  • See [Is there a performance impact when using ToList()?](http://stackoverflow.com/questions/15516462/is-there-a-performance-impact-when-calling-tolist). – mason Apr 25 '14 at 20:00
  • The simplest thing would be to simply remove "ToList()" although that may or may not fix your problem. Calling ToList() causes the entire IEnumerable/IQueryable to be resolved at once instead of lazily. – Casey Apr 25 '14 at 20:04

2 Answers2

3

You might be able to speed this up by dropping the ToList() from the orglist query. This uses deferred execution, rather than pulling all the records for the org list. However, if there is no match on the query that calls Contains(), it will still have to load everything.

public List<decimal> GetOrgSolution()
{
    //Need to use USER id. but we have EMPNO in session. 
    var Users = db.CRM_USERS.Where(c => c.ID == SessionCurrentUser.ID || RelOrgPerson.Contains(c.EMPNO.Value)).Select(c => c.ID);

    //Get the organization list regarding to HR organization
    var OrgList = db.CRM_SOLUTION_ORG.Where(c => c.USER_ID == SessionCurrentUser.ID || Users.Contains(c.USER_ID.Value)).Select(c => c.ID);

    //Get related solutions ID with the OrgList
    List<decimal> SolutionList = db.CRM_SOLUTION_OWNER.Where(p => OrgList.Contains(p.SOLUTION_ORG_ID.Value)).Select(c => (decimal)c.SOLUTION_ID).Distinct().ToList();

    return SolutionList;
}
Jon B
  • 51,025
  • 31
  • 133
  • 161
  • Also i am getting some info on view like SolutionList.departmant.Name, should i use Include Method to eager loading for better loading time ? – umki Apr 25 '14 at 20:01
  • I don't think the change from Contains to Any is necessary. – Jeremy Cook Apr 25 '14 at 20:01
  • @JeremyCook - you're right. I had it in my head that only `IList` had `Contains`. Shame on me. – Jon B Apr 25 '14 at 20:03
  • If i remove toList(), i need to remove List SolutionList, and i can write var SolutionList .. But what should my return type be ? I mean , what do i need to write for" public List GetOrgSolution()" – umki Apr 25 '14 at 20:05
  • @umki - sorry, I don't understand your followup question. If what you're saying is that you need more than just the solution ID, then you may need to return an object that contains everything you need. – Jon B Apr 25 '14 at 20:05
  • 1
    @umki you can keep `ToList` on the last query. I'm just saying to remove it from the `var OrgList = ...` query. – Jon B Apr 25 '14 at 20:06
  • Seems to me he might want `ToList` on the `Users` and on the `OrgList`. Otherwise, every time `Users.Contains` is called, the query is going to be re-executed. Same with when `OrgList.Contains` is called. – Jim Mischel Apr 25 '14 at 21:03
2

Unless the lists you're working with are really huge, it's highly unlikely that calling ToList is the major bottleneck in your code. I'd be much more inclined to suspect the database (assuming you're doing LINQ-to-SQL). Or, your embedded Contains calls. You have, for example:

db.CRM_SOLUTION_ORG..Where(
    c => c.USER_ID == SessionCurrentUser.ID || Users.Contains(c.USER_ID.Value))

So for every item in db.CRM_SOLUTION_ORG that fails the test against SessionCurrentUser, you're going to do a sequential search of the Users list.

Come to think of it, because Users is lazily evaluated, you're going to execute that Users query every time you call Users.Contains. It looks like your code would be much more efficient in this case if you called ToList() on the Users. That way the query is only executed once.

And you probably should keep the ToList() on the OrgList query. Otherwise you'll be re-executing that query every time you call OrgList.Contains.

That said, if Users or OrgList could have a lot of items, then you'd be better off turning them into HashSets so that you get O(1) lookup rather than O(n) lookup.

But looking at your code, it seems like you should be able to do all of this with a single query using joins, and let the database server take care of it. I don't know enough about Linq to SQL or your data model to say for sure, but from where I'm standing it sure looks like a simple joining of three tables.

Jim Mischel
  • 131,090
  • 20
  • 188
  • 351