6

We experienced some slowness in our code opening a form and it was possibly due to a for loop with a break that was taking a long time to execute. I switched this to an IEnumerable.Any() and saw the form open very quickly. I am now trying to figure out if making this change alone increased performance or if it was accessing the ProductIDs property more efficiently. Should this implementation be faster, and if so, why?

Original Implementation:

public bool ContainsProduct(int productID) {
    bool containsProduct = false;
    for (int i = 0; i < this.ProductIDs.Length; i++) {
        if (productID == this.ProductIDs[i]) {
            containsProduct = true;
            break;
        }
    }
    return containsProduct;
}

New Implementation:

public bool ContainsProduct(int productID) {
    return this.ProductIDs.Any(t => productID == t);
}
Ian R. O'Brien
  • 6,682
  • 9
  • 45
  • 73

6 Answers6

9

Call this an educated guess:

this.ProductIDs.Length

This probably is where the slowness lies. If the list of ProductIDs gets retrieved from database (for example) on every iteration in order to get the Length it would indeed be very slow. You can confirm this by profiling your application.

If this is not the case (say ProductIDs is in memory and Length is cached), then both should have an almost identical running time.

Oded
  • 489,969
  • 99
  • 883
  • 1,009
  • So the suggestion to prove this is to access ProductIDs.Length once and store it in a local variable, then put that variable in your for loop. – Adam Houldsworth Jun 15 '11 at 14:17
  • @Adam - Yes, _if_ that's the cause of the slowness. – Oded Jun 15 '11 at 14:18
  • @Oded yeah, I was merely commenting so the OP could see a possible way to exercise your idea. – Adam Houldsworth Jun 15 '11 at 14:19
  • This is the only possible solution, imo. I more likely suspect that this isn't the only change that was made or some external factor applies. – Marc Jun 15 '11 at 14:21
  • This was actually the problem in my case, although ProductIDs wasn't coming from a database. There was a lot of horrible code associated with that property. I marked Aliostad's as the answer since it answered the more general question of will a .Any provide greater performance than a for-break. Will ProductIDs be accessed more times in a for loop than a .Any? – Ian R. O'Brien Jun 15 '11 at 18:37
8

First implementation is slightly faster (enumeration is slightly slower than for loop). Second one is a lot more readable.


UPDATE

Oded's answer is possibly correct and well done for spotting it. The first one is slower here since it involves database roundtrip. Otherwise, it is slightly faster as I said.


UPDATE 2 - Proof

Here is a simple code showing why first one is faster:

    public static void Main()
    {

        int[] values = Enumerable.Range(0, 1000000).ToArray();

        int dummy = 0;
        Stopwatch stopwatch = new Stopwatch();

        stopwatch.Start();
        for (int i = 0; i < values.Length; i++)
        {
            dummy *= i;
        }
        stopwatch.Stop();
        Console.WriteLine("Loop took {0}", stopwatch.ElapsedTicks);

        dummy = 0;
        stopwatch.Reset();
        stopwatch.Start();
        foreach (var value in values)
        {
            dummy *= value;         
        }
        stopwatch.Stop();
        Console.WriteLine("Iteration took {0}", stopwatch.ElapsedTicks);

        Console.Read();
    }

Here is output:

Loop took 12198

Iteration took 20922

So loop is twice is fast as iteration/enumeration.

Community
  • 1
  • 1
Aliostad
  • 80,612
  • 21
  • 160
  • 208
  • 2
    I think the *slightly* you are referring to would be ridiculous to optimize against. – Marc Jun 15 '11 at 14:16
  • I offer facts, no more. I personally prefer second one. – Aliostad Jun 15 '11 at 14:18
  • However, the two are far from identical. See the call to `this.ProductIDs.Length` that occurs on every loop iteration and not at all in the `LINQ` query. – Oded Jun 15 '11 at 14:20
  • I think `foreach` and enumeration actually gives the compiler more optimization room to work with... at least that's what I remember from reading Effective C# way back in the .NET 1.1 days. – Domenic Jun 15 '11 at 14:20
  • @Aliostad, I'm not disagreeing, just commenting for followup readers to understand the scope of the speed difference. ;) – Marc Jun 15 '11 at 14:23
  • See my updates. I tested and first one faster (although user's problem seems to be something else) – Aliostad Jun 15 '11 at 14:32
  • One run does not a profile make. Some runs, I bet the for loop comes out taking more ticks. May want to run this millions of times and take the average to accurately profile your proof. – Marc Jun 15 '11 at 15:36
  • Do it and then post the code. I have done, so you could too. Let's talk evidence... – Aliostad Jun 15 '11 at 15:40
4

I think they would be more or less identical. I usually refer to Jon Skeet's Reimplementing LINQ to Objects blog series to get an idea of how the extension methods work. Here's the post for Any() and All()

Here's the core part of Any() implementation from that post

public static bool Any<TSource>( 
    this IEnumerable<TSource> source, 
    Func<TSource, bool> predicate) 
{ 
   ...

    foreach (TSource item in source) 
    { 
        if (predicate(item)) 
        { 
            return true; 
        } 
    } 
    return false; 
} 
Bala R
  • 107,317
  • 23
  • 199
  • 210
1

This post assumes that ProductIDs is a List<T> or an array. So I'm talking about Linq-to-objects.

Linq is usually slower but shorter/more readable than conventional loop based code. A factor of 2-3 depending on what you're doing is typical.

Can you refactor your code to make this.ProductIDs a HashSet<T>? Or at least sort the array so you can use a binary search. Your problem is that you're performing a linear search, which is slow if there are many products.

CodesInChaos
  • 106,488
  • 23
  • 218
  • 262
0

I think the below implementation would be a little faster than the corresponding linq implementation, but very minor though

public bool ContainsProduct(int productID) {
    var length = this.ProductIDs.Length;

    for (int i = 0; i < length; i++) {
        if (productID == this.ProductIDs[i]) {
            return true;
        }
    }

    return false;
}
Mahesh Velaga
  • 21,633
  • 5
  • 37
  • 59
0

The difference will be generally in memory usage then speed.

But generally you should use for loop when you know that you will be using all elements of array in other cases you should try to use while or do while.

I think that this solution use minimum resources

int i = this.ProductIDs.Length - 1;

while(i >= 0) {
 if(this.ProductIDs[i--] == productId) {
   return true;
 }
}

return false;