50

I have two classes that perform date date range data fetching for particular days.

public class IterationLookup<TItem>
{
    private IList<Item> items = null;

    public IterationLookup(IEnumerable<TItem> items, Func<TItem, TKey> keySelector)
    {
        this.items = items.OrderByDescending(keySelector).ToList();
    }

    public TItem GetItem(DateTime day)
    {
        foreach(TItem i in this.items)
        {
           if (i.IsWithinRange(day))
           {
               return i;
           }
        }
        return null;
    }
}


public class LinqLookup<TItem>
{
    private IList<Item> items = null;

    public IterationLookup(IEnumerable<TItem> items, Func<TItem, TKey> keySelector)
    {
        this.items = items.OrderByDescending(keySelector).ToList();
    }

    public TItem GetItem(DateTime day)
    {
        return this.items.FirstOrDefault(i => i.IsWithinRange(day));
    }
}

Then I do speed tests which show that Linq version is about 5 times slower. This would make sense when I would store items locally without enumerating them using ToList. This would make it much slower, because with every call to FirstOrDefault, linq would also perform OrderByDescending. But that's not the case so I don't really know what's going on. Linq should perform very similar to iteration.

This is the code excerpt that measures my timings

IList<RangeItem> ranges = GenerateRanges(); // returns List<T>

var iterLookup = new IterationLookup<RangeItems>(ranges, r => r.Id);
var linqLookup = new LinqLookup<RangeItems>(ranges, r => r.Id);

Stopwatch timer = new Stopwatch();

timer.Start();
for(int i = 0; i < 1000000; i++)
{
    iterLookup.GetItem(GetRandomDay());
}
timer.Stop();
// display elapsed time

timer.Restart();
for(int i = 0; i < 1000000; i++)
{
    linqLookup.GetItem(GetRandomDay());
}
timer.Stop();
// display elapsed time

Why do I know it should perform better? Because when I write a very similar code without using these lookup classes, Linq performs very similar to foreach iterations...

// continue from previous code block

// items used by both order as they do in classes as well
IList<RangeItem> items = ranges.OrderByDescending(r => r.Id).ToList();

timer.Restart();
for(int i = 0; i < 1000000; i++)
{
    DateTime day = GetRandomDay();
    foreach(RangeItem r in items)
    {
        if (r.IsWithinRange(day))
        {
            // RangeItem result = r;
            break;
        }
    }
}    
timer.Stop();
// display elapsed time

timer.Restart();
for(int i = 0; i < 1000000; i++)
{
   DateTime day = GetRandomDay();
   items.FirstOrDefault(i => i.IsWithinRange(day));
}
timer.Stop();
// display elapsed time

This is by my opinion highly similar code. FirstOrDefault as much as I know also iterate for only as long until it gets to a valid item or until it reaches the end. And this is somehow the same as foreach with break.

But even iteration class performs worse than my simple foreach iteration loop which is also a mystery because all the overhead it has is the call to a method within a class compared to direct access.

Question

What am I doing wrong in my LINQ class that it performs so very slow?
What am I doing wrong in my Iteration class so it performs twice as slow as direct foreach loop?

Which times are being measured?

I do these steps:

  1. Generate ranges (as seen below in results)
  2. Create object instances for IterationLookup, LinqLookup (and my optimized date range class BitCountLookup which is not part of discussion here)
  3. Start timer and execute 1 million lookups on random days within maximum date range (as seen in results) by using previously instantiated IterationLookup class.
  4. Start timer and execute 1 million lookups on random days within maximum date range (as seen in results) by using previously instantiated LinqLookup class.
  5. Start timer and execute 1 million lookups (6 times) using manual foreach+break loops and Linq calls.

As you can see object instantiation is not measured.

Appendix I: Results over million lookups

Ranges displayed in these results don't overlap, which should make both approaches even more similar in case LINQ version doesn't break loop on successful match (which it highly likely does).

Generated Ranges:

ID Range        000000000111111111122222222223300000000011111111112222222222
                123456789012345678901234567890112345678901234567890123456789
09 22.01.-30.01.                     |-------|
08 14.01.-16.01.             |-|
07 16.02.-19.02.                                              |--|
06 15.01.-17.01.              |-|
05 19.02.-23.02.                                                 |---|
04 01.01.-07.01.|-----|
03 02.01.-10.01. |-------|
02 11.01.-13.01.          |-|
01 16.01.-20.01.               |---|
00 29.01.-06.02.                            |-------|

Lookup classes...

- Iteration: 1028ms
- Linq: 4517ms   !!! THIS IS THE PROBLEM !!!
- BitCounter: 401ms

Manual loops...

- Iter: 786ms
- Linq: 981ms
- Iter: 787ms
- Linq: 996ms
- Iter: 787ms
- Linq: 977ms
- Iter: 783ms
- Linq: 979ms

Appendix II: GitHub:Gist code to test for yourself

I've put up a Gist so you can get the full code yourself and see what's going on. Create a Console application and copy Program.cs into it an add other files that are part of this gist.

Grab it here.

Appendix III: Final thoughts and measurement tests

The most problematic thing was of course LINQ implementatino that was awfully slow. Turns out that this has all to do with delegate compiler optimization. LukeH provided the best and most usable solution that actually made me try different approaches to this. I've tried various different approaches in the GetItem method (or GetPointData as it's called in Gist):

  1. the usual way that most of developers would do (and is implemented in Gist as well and wasn't updated after results revealed this is not the best way of doing it):

    return this.items.FirstOrDefault(item => item.IsWithinRange(day));
    
  2. by defining a local predicate variable:

    Func<TItem, bool> predicate = item => item.IsWithinRange(day);
    return this.items.FirstOrDefault(predicate);
    
  3. local predicate builder:

    Func<DateTime, Func<TItem, bool>> builder = d => item => item.IsWithinRange(d);
    return this.items.FirstOrDefault(builder(day));
    
  4. local predicate builder and local predicate variable:

    Func<DateTime, Func<TItem, bool>> builder = d => item => item.IsWithinRange(d);
    Func<TItem, bool> predicate = builder(day);
    return this.items.FirstOrDefault(predicate);
    
  5. class level (static or instance) predicate builder:

    return this.items.FirstOrDefault(classLevelBuilder(day));
    
  6. externally defined predicate and provided as method parameter

    public TItem GetItem(Func<TItem, bool> predicate)
    {
        return this.items.FirstOrDefault(predicate);
    }
    

    when executing this method I also took two approaches:

    1. predicate provided directly at method call within for loop:

      for (int i = 0; i < 1000000; i++)
      {
          linqLookup.GetItem(item => item.IsWithinRange(GetRandomDay()));
      }
      
    2. predicate builder defined outside for loop:

      Func<DateTime, Func<Ranger, bool>> builder = d => r => r.IsWithinRange(d);
      for (int i = 0; i < 1000000; i++)
      {
          linqLookup.GetItem(builder(GetRandomDay()));
      }
      

Results - what performs best

For comparison when using iteration class, it takes it approx. 770ms to execute 1 million lookups on randomly generated ranges.

  1. 3 local predicate builder turns out to be best compiler optimized so it performs almost as fast as usual iterations. 800ms.

  2. 6.2 predicate builder defined outside for loop: 885ms

  3. 6.1 predicate defined within for loop: 1525ms

  4. all others took somewhere between 4200ms - 4360ms and are thus considered unusable.

So whenever you use a predicate in externally frequently callable method, define a builder and execute that. This will yield best results.

The biggest surprise to me about this is that delegates (or predicates) may be this much time consuming.

Community
  • 1
  • 1
Robert Koritnik
  • 103,639
  • 52
  • 277
  • 404
  • 2
    Are you timing this on a *Release* build outside of the IDE? – James Michael Hare Nov 21 '11 at 15:32
  • I think the performance difference comes from the cast operation the BCL does to determine if the given collection implements `IList`. That optimization is questionable, since you always want to get the first item anyway (instead when doing `LastOrDefault`). – Steven Nov 21 '11 at 15:36
  • @JamesMichaelHare: These are all in **Debug mode without debugger being attached**. Let me check in release mode as well... Ok. Here're results: Release performs faster but anomaly still exists in terms of 5x factor. – Robert Koritnik Nov 21 '11 at 15:36
  • How do you initialize your lookup classes? Just guessing, but if you had something like `new LinqLookup(items.ToList(), ...)` then you would be calling `ToList()` twice. Why not just dismiss the `ToList()` in the constructor? `this.items = items.OrderByDescending(keySelector);`. – Olivier Jacot-Descombes Nov 21 '11 at 15:36
  • @OlivierJacot-Descombes: object instantiation is not included in timing. I create once and reuse it. The actual calls being measured here are `GetItem` method calls. – Robert Koritnik Nov 21 '11 at 15:38
  • @Robert: Could you post more of your code, including the benchmarking stuff, so that we can try to replicate and/or diagnose more accurately? – LukeH Nov 21 '11 at 15:41
  • @everyone: I've edited my question to include my timing methodology that explains my measurements. As you can see I'm not including object instantiation in my timings. Pure lookups are measured, nothing else. – Robert Koritnik Nov 21 '11 at 15:45
  • @OlivierJacot-Descombes: Actually what you're implying it would make it more prone to errors because inner class would depend on how you provide your items. By using `ToList()` within constructor eliminates any other Linq manipulation outside class before calling constructor. – Robert Koritnik Nov 21 '11 at 15:48
  • Is your manual code inline? I mean: Is the test code implemented directly in the test loop? The difference might come from the method call of `GetItem`. – Olivier Jacot-Descombes Nov 21 '11 at 15:55
  • @LukeH: check my edited question... More code included. – Robert Koritnik Nov 21 '11 at 16:00
  • @OlivierJacot-Descombes: Yes my manual code is inline (as I edited my question to reflect that fact). And that most likely explains the bit slower class usage when using `foreach` looping. Still it doesn't explain the huge discrepancy when using LINQ manually and LINQ in class. – Robert Koritnik Nov 21 '11 at 16:02
  • Like James i can't reproduce that large timing difference. It might be related to an implementation detail you have not shown yet. – Jan Nov 21 '11 at 16:15
  • may be the FirstOrDefault() implementation calls the First() method, with a try catch, then the exception throw could explain the difference ? – tahir Nov 21 '11 at 16:24
  • 1
    @tahir: FirstOrDefault() does not throw if not found. Only throws on null arguments. – James Michael Hare Nov 21 '11 at 16:25
  • @RobertKoritnik: What framework version? I wonder if something got optimized between 3.5 and 4.0... – James Michael Hare Nov 21 '11 at 16:26
  • @tahir: that shouldn't be the case, because I changed my code to have full range coverage so `FirstOrDefault` always returns an element without defaulting (or in your speculation throwing exceptions). Discrepancy is still there though... – Robert Koritnik Nov 21 '11 at 16:32
  • @JamesMichaelHare: Edited my tags to include net4. (although it will be running in 3.5 in production). I've also tried compiling in 3.5 but there's no difference whatsoever. – Robert Koritnik Nov 21 '11 at 16:34
  • @RoberKoritnik: Scratch that, even in 3.5 I'm not seeing the difference. – James Michael Hare Nov 21 '11 at 16:35
  • 1
    @RobertKoritnik: Maybe If you could post the whole code snippet including the data generation. We are not seeing the 5x difference, so there may be some problem in the way the data is being generated or the test being run. – James Michael Hare Nov 21 '11 at 16:37
  • @RobertKoritnik: What's GetRandomDay() look like? – James Michael Hare Nov 21 '11 at 16:44
  • @JamesMichaelHare: `GetRandomDay` **is not an issue** whatsoever, because it's used in all loops, so it means that its time is always the same (doesn't really matter whether it's fast or slow it's always the same amount of time). It uses `Random` to generate dates within the whole date range (1st Jan to 29th Feb). But as said. It's not an issue. – Robert Koritnik Nov 22 '11 at 06:39
  • Have tried your sample and can't see that difference. Linq is a bit slower than Iteration on my system. – Jan Nov 22 '11 at 08:31
  • @Jan: Have you tried the code provided in the Gist or just the code provided here? – Robert Koritnik Nov 22 '11 at 13:22
  • Yes, i have tried your Gist and see only a minor performance breakdown – Jan Nov 22 '11 at 13:40
  • @RobertKoritnik: The reason I wanted to see GetRandomDay() was to see if there could be some possible artifact in the way it generates random dates that could affect your outcome later in the tests. – James Michael Hare Nov 22 '11 at 14:58
  • @JamesMichaelHare: You've probably checked Gist to realise that's not the case. But it turns out that LukeH helped lots. This has all to do with delegate compiler optimization. I'll edit my original question with the measurement outcome and best solution to achieve best results. I'm sure you'll all be very interested in them. – Robert Koritnik Nov 22 '11 at 15:53
  • @RobertKoritnik: Sounds good. I can't check gist from where I currently am (firewall), but I'm very interested to see results. – James Michael Hare Nov 22 '11 at 16:05
  • @JamesMichaelHare: Check results up in the post. – Robert Koritnik Nov 22 '11 at 16:46
  • I don't get why a local predicate builder performs better than a predicate builder defined outside the for loop. When the predicate builder is defined locally as in #3 , doesn't that predicate being build in every iteration of the loop? Maybe I'm missing something here. – phabtar Feb 17 '15 at 08:12

3 Answers3

14

Sometimes LINQ appears slower because the generation of delegates in a loop (especially a non-obvious loop over method calls) can add time. Instead, you may want to consider moving your finder out of the class to make it more generic (like your key selector is on construction):

public class LinqLookup<TItem, TKey>
{
    private IList<Item> items = null;

    public IterationLookup(IEnumerable<TItem> items, Func<TItem, TKey> keySelector)
    {
        this.items = items.OrderByDescending(keySelector).ToList();
    }

    public TItem GetItem(Func<TItem, TKey> selector)
    {
        return this.items.FirstOrDefault(selector);
    }
}

Since you don't use a lambda in your iterative code, this can be a bit of a difference since it has to create the delegate on each pass through the loop. Usually, this time is insignificant for every-day coding, and the time to invoke the delegate is no more expensive than other method calls, it's just the delegate creation in a tight loop that can add that little bit of extra time.

In this case, since the delegate never changes for the class, you can create it outside of the code you are looping through and it would be more efficient.

Update:

Actually, even without any optimization, compiling in release mode on my machine I do not see the 5x difference. I just performed 1,000,000 lookups on an Item that only has a DateTime field, with 5,000 items in the list. Of course, my data, etc, are different, but you can see the times are actually really close when you abstract out the delegate:

iterative : 14279 ms, 0.014279 ms/call

linq w opt : 17400 ms, 0.0174 ms/call

These time differences are very minor and worth the readability and maintainability improvements of using LINQ. I don't see the 5x difference though, which leads me to believe there's something we're not seeing in your test harness.

Community
  • 1
  • 1
James Michael Hare
  • 37,767
  • 9
  • 73
  • 83
  • How come there's so much difference between using LinqLookup and manual LINQ calls that also don't reuse the predicate? At least they should yield similar results (as foreach iterations do)... – Robert Koritnik Nov 21 '11 at 16:05
  • 3
    In your manual call, the compiler *may* realize it's the same lambda inside the loop and optimize, whereas in your class, it has no idea that that method will be called 1 million times and thus can't optimize. – James Michael Hare Nov 21 '11 at 16:07
  • **Another question:** How am I supposed to save the predicate locally, when it also takes an additional parameter named `day`? I can't create a `Func`, because `FirstOrDefault` won't know how to use it. – Robert Koritnik Nov 21 '11 at 16:09
  • @RobertKoritnik: Updated my post. I'm not seeing your time difference (I'm on 4.0 Framework), also updated on moving out the projection. – James Michael Hare Nov 21 '11 at 16:30
  • @Gent: Thanks! Wish we could reproduce the 5x time issue though. – James Michael Hare Nov 21 '11 at 16:36
  • I still can't see how to put predicate outside, since my items are inside, and I have to pass through a date along with it as well. So basically I need to pass in `Finc` which won't be understood by `FirstOrDefault`... In your case the `DateTime` parameter isn't needed since you have it as a property inside your class (as you're not dealing with ranges). – Robert Koritnik Nov 21 '11 at 16:44
  • 1
    @RobertKoritnik: just call the lambda from your test loop `i => i.IsWithinRange(date)` (instead of directly in your class), but I don't think this is your 5x issue, it is a minor optimization but one of the reasons why LINQ is slightly slower. The 5x slower is another matter entirely which doesn't seem to be LINQ related. – James Michael Hare Nov 21 '11 at 16:54
  • @JamesMichaelHare: Exactly. I've checked all enumerations of the set it it doesn't seem that I would be doing more than O(n) anywhere, that's the reason that's confusing me. I'll have to dig a bit deeper I guess. – Robert Koritnik Nov 21 '11 at 20:46
  • @RobertKoritnik: Can you post the full code including the GetRandomDate(), GetRanges(), etc? – James Michael Hare Nov 21 '11 at 20:51
  • You're right that putting lambda out of the class and inside the loop doesn't change results too much. It's a matter of about 5% really. – Robert Koritnik Nov 22 '11 at 06:42
  • @JamesMichaelHare: I still +1'd your post, because it seems you've been pushing this question (and solution) forward so we came to a usable result. Thanks again James. But I had to accept Luke's answer for obvious reasons. – Robert Koritnik Nov 22 '11 at 20:14
  • @RobertKoritnik: Hey, no problem! Thanks for the +1, much appreciated! Luke did have the true answer, and I'm glad because I just wasn't seeing the 5x difference in my local tests (unfortunately I didn't see the code update until it was solved, but hey that's life :-) ) – James Michael Hare Nov 22 '11 at 20:19
9

Further to Gabe's answer, I can confirm that the difference appears to be caused by the cost of re-constructing the delegate for every call to GetPointData.

If I add a single line to the GetPointData method in your IterationRangeLookupSingle class then it slows right down to the same crawling pace as LinqRangeLookupSingle. Try it:

// in IterationRangeLookupSingle<TItem, TKey>
public TItem GetPointData(DateTime point)
{
    // just a single line, this delegate is never used
    Func<TItem, bool> dummy = i => i.IsWithinRange(point);

    // the rest of the method remains exactly the same as before
    // ...
}

(I'm not sure why the compiler and/or jitter can't just ignore the superfluous delegate that I added above. Obviously, the delegate is necessary in your LinqRangeLookupSingle class.)

One possible workaround is to compose the predicate in LinqRangeLookupSingle so that point is passed to it as an argument. This means that the delegate only needs to be constructed once, not every time the GetPointData method is called. For example, the following change will speed up the LINQ version so that it's pretty much comparable with the foreach version:

// in LinqRangeLookupSingle<TItem, TKey>
public TItem GetPointData(DateTime point)
{
    Func<DateTime, Func<TItem, bool>> builder = x => y => y.IsWithinRange(x);
    Func<TItem, bool> predicate = builder(point);

    return this.items.FirstOrDefault(predicate);
}
Community
  • 1
  • 1
LukeH
  • 263,068
  • 57
  • 365
  • 409
  • This solution actually speeds things up considerably (almost as fast as iterative approach). But instead of defining two predicates, just define a builder and call it within `FirstOrDefault`); – Robert Koritnik Nov 22 '11 at 15:25
  • 1
    @Robert: Yep, that's what I had originally, but I split it into two just to make it explicit what was going on. – LukeH Nov 22 '11 at 15:27
  • Check measurement results that I put in original question above. It may surprise you, but your answer led me to best solution. – Robert Koritnik Nov 22 '11 at 16:47
  • @Robert: The results are a bit surprising. I can't help feeling that the compiler should be able to optimise your original LINQ class (after all, it can optimise the manual LINQ loop and it can optimise the "builder" versions). This *feels* like a compiler bug to me, although it also feels like I've overlooked some important detail and just stumbled on a lucky workaround. I expect there's a proper explanation and justification for this somewhere in the C# spec. – LukeH Nov 22 '11 at 17:02
  • @JonSkeet: Hey Luke, unless Jon Skeet add his own 2 cents about this question, we'll have to think that compiler optimization is not optimizing as we think it should. :)Jon would almost certainly know reasons behind this code. – Robert Koritnik Nov 22 '11 at 20:13
  • @LukeH: I wonder is there an indicator when we can be certain that our delegate/predicate/lambda won't be compiler optimized (or whatever happened here... reusing a generated lambda instead of regenerating every time maybe)? I would like to come up with a solution that will help me write faster code, because these differences are significant! – Robert Koritnik Nov 23 '11 at 10:16
6

Assume you have a loop like this:

for (int counter = 0; counter < 1000000; counter++)
{
    // execute this 1M times and time it 
    DateTime day = GetRandomDay(); 
    items.FirstOrDefault(i => i.IsWithinRange(day)); 
}

This loop will create 1,000,000 lambda objects in order for the i.IsWithinRange call to access day. After each lambda creation, the delegate that calls i.IsWithinRange gets invoked on average 1,000,000 * items.Length / 2 times. Both of those factors do not exist in your foreach loop, which is why the explicit loop is faster.

Gabe
  • 84,912
  • 12
  • 139
  • 238
  • Ok how do you explain the **huge** timing difference between class and manual LINQ usage (regardless of `foreach` looping)? – Robert Koritnik Nov 21 '11 at 16:06
  • @Robert: I don't understand what "class and manual LINQ usage" means. Can you tell me what that means and what the timing differences are? – Gabe Nov 21 '11 at 16:31