trying to optimize if/else condition slows down program

Question

I am currently trying to optimize a .net application with the help of the VS-Profiling tools.

One function, which gets called quite often, contains the following code:

if (someObjectContext.someObjectSet.Where(i => i.PNT_ATT_ID == tmp_ATT_ID).OrderByDescending(i => i.Position).Select(i => i.Position).Count() == 0)
{
    lastPosition = 0;
}
else
{
    lastPosition = someObjectContext.someObjectSet.Where(i => i.PNT_ATT_ID == tmp_ATT_ID).OrderByDescending(i => i.Position).Select(i => i.Position).Cast<int>().First();
}

Which I changed to something like this:

var relevantEntities = someObjectContext.someObjectSet.Where(i => i.PNT_ATT_ID == tmp_ATT_ID).OrderByDescending(i => i.Position).Select(i => i.Position);
if (relevantEntities.Count() == 0)
{
    lastPosition = 0;
}
else
{
    lastPosition = relevantEntities.Cast<int>().First();
}

I was hoping that the change would speed up the method a bit, as I was unsure wether the compiler would notice that the query is done twice and cache the results.

To my surprise the execution time (the number of inklusive samplings) of the method has not decreased, but even increased by 9% (according to the profiler)

Can someone explain why this is happening?

Regardless of performance, your second example is more better written and more maintainable code per the DRY rule. — JohnFx, Jun 19 '13 at 14:19

Sergey Berezovskiy · Answer 1 · 2013-06-19T14:57:04.780

You can use Max() to get maximum position instead of ordering and taking first item, and DefaultIfEmpty() to provide default value (zero for int) if there are no entities matching your condition. Btw you can provide custom default value to return if sequence is empty.

lastPosition = someObjectContext.someObjectSet
                                .Where(i => i.PNT_ATT_ID == tmp_ATT_ID)
                                .Select(i => i.Position)
                                .Cast<int>()
                                .DefaultIfEmpty() 
                                .Max();

Thus you will avoid executing two queries - one for defining if there is any positions, and another for getting latest position.

score 3 · Accepted Answer · edited May 23 '17 at 12:29

I was hoping that the change would speed up the method a bit, as I was unsure whether the compiler would notice that the query is done twice and cache the results.

It will not. In fact it cannot. The database might not return the same results for the two queries. It's entirely possible for a result to be added or removed after the first query and before the second. (Making this code not only inefficient, but potentially broken if that were to happen.) Since it's entirely possible that you want two queries to be executed, knowing that the results could differ, it's important that the results of the query not be re-used.

The important point here is the idea of deferred execution. relevantEntities is not the results of a query, it's the query itself. It's not until the IQueryable is iterated (by a method such as Count, First, a foreach loop, etc.) that the database will be queried, and each time you iterate the query it will perform another query against the database.

In your case you can just do this:

var lastPosition = someObjectContext.someObjectSet
    .Where(i => i.PNT_ATT_ID == tmp_ATT_ID)
    .OrderByDescending(i => i.Position)
    .Select(i => i.Position)
    .Cast<int>()
    .FirstOrDefault();

This leverages the fact that the default value of an int is 0, which is what you were setting the value to in the event that there was not match before.

Note that this is a query that is functionally the same as yours, it just avoids executing it twice. An even better query would be the one suggested by lazyberezovsky in which you leveraged Max rather than ordering and taking the first. If there is an index on that column there wouldn't be much of a difference, but if there's not a an index ordering would be a lot more expensive.

This is actually the correct answer. I was in the process of writing something similar, but you did it first. — Eldritch Conundrum, Jun 19 '13 at 15:02
+1 from me simple `FirstOrDefault()` also will do the job in this case :) — Sergey Berezovskiy, Jun 19 '13 at 15:09
@lazyberezovsky Yeah, as I said the key point here is whether there is an index on the column. That will determine if there's a noticeable performance difference in ordering vs not. — Servy, Jun 19 '13 at 15:10
Thanks. I have tried both versions and this one was the fasted. The one with `DefaultIfempty().Max()` was even a little slower then the original code. I'm starting to believe that my server might be influenced by sun flares or something similar which causes him to be faster or slower randomly ;) — ChNissen, Jun 19 '13 at 16:18

trying to optimize if/else condition slows down program

2 Answers2