views:

174

answers:

4

I'm thinking of replacing a lot of inline foreaches with Linq and in so doing will make new methods, e.g. Current:

foreach(Item in List)
{
    Statement1
    Statement2
    Statement3
}

Idea:

List.Foreach(Item => Method(Item))

Obviously Method() contains Statement1..3

Is this good practise or is calling a method thousands of times going to degrade performance? My Lists have 10,000-100,000 elements.

+3  A: 

Well, for one thing you can probably make the ForEach statement more efficient using a method group conversion

List.ForEach(Method);

That's removed one level of indirection.

Personally though, I don't think it's a good idea. The first approach is more readable, and likely to perform about as well. What's the advantage of using List<T>.ForEach here?

Eric Lippert talks about this more in an excellent blog post. I would use List<T>.ForEach if you already had a delegate you wanted to execute against each element, but I wouldn't introduce a delegate and an extra method just for the sake of it.

In terms of efficiency, I wouldn't expect to see much difference. The first form may perform a little better as it doesn't have the indirection of the delegate call - but the second form may be more efficient if the iteration loop within ForEach makes use of the fact that it has access to the internal data structures of the List<T>. I very much doubt you'll notice it either way. You could try to measure it if you're really bothered, of course.

Jon Skeet
@Jon Skeet: Perhaps you are unaware that *someone* once did some pretty extensive performance comparisons on the various methods to enumerate a `List<T>`. Hope that helps. :) http://msmvps.com/blogs/jon_skeet/archive/2006/01/20/foreachperf.aspx
Ani
@Ani: Gosh, I'd completely forgotten about that. Have just rerun the code with .NET 4, and the "language foreach" version seems to have improved somewhat.
Jon Skeet
What is a "method group conversion" and how does it help?
Ian Ringrose
@Ian: It's the conversion from the name of a method to a delegate. The lambda expression will end up creating an extra method just to call `Method`. Method group conversions are what allow things like `button.Click += ClickHandler;`
Jon Skeet
re "method group conversion", Thanks, I would had hoped the C# compiler would optimize this it’s self!
Ian Ringrose
+1  A: 

If your motivation for considering the change is that the three statements in the body are too complicated, then I'd probably use ordinary foreach, but refactor the body to a method:

foreach(var item in List) 
  Method(item);

If the code in the body isn't complicated, then I'd agree with Jon that there is no good reason for using ForEach (it doesn't make the code more readable/declarative in any way).

I generally don't like using "LINQ-like" constructs to do imperative processing at the end of a LINQ query. I think that using foreach more clearly states that you're finished with querying data and you're doing some processing now.

Tomas Petricek
A: 

I've always thought that you should write your code for readability first because the compiler and CLR do an exceptional job at optimisation. If you find that through benchmarking, that this code could be executed more quickly, then have a look at other options by all means.

E.g. for loops are quicker than foreach(), as they use array offsets which are internally optimised in the CLR.

But doesn't a List.ForEach () surely does a foreach ( ) anyway, so you are just giving the work to another method, rather than doing it yourself.

Strictly speaking, introducing more method calls will actually slow your code down on the first pass, because the CLR will JIT-compile methods as they are called, although subsequent calls to the method will not.

So my advice would be stick to writing readable code, then go from there if you can prove that this is a bottleneck of the system.

Dominic Zukiewicz
I'm not sure where you get the idea that the CLR does such an exceptional job at optimisation. Optimization isn't easy, particularly with things like multi-threading and side-effects (which C# includes), and my experience indicates that the CLR does reasonable well at eliminating overhead, but that you should not expect it to actually be able to rearrange code to even trivially "equivalent" variants that are faster. Write for readability because performance often doesn't matter, not because you expect miracles from the optimizer.
Eamon Nerbonne
I assume the goal is 2 fold: Less code which results in performance benefits. I would still go for readability first whether that be refactoring, or writing more lines of code to help maintainability. But prove it is a bottleneck first before trying to optimise.
Dominic Zukiewicz
A: 

I'm totally agree with Jon Skeet's answer. But since we are talking about ForEach performance, I have something addtional to your question. Be aware of that if your Statement 1~3 is not relative with each other, that is:

foreach(Item in List) 
{ 
   DoSomething();
   DoAnotherThing(); 
   DoTheLastThing();
} 

The code above probably has a worse performance than the following:

foreach(Item in List) 
{ 
   DoSomething();
} 
foreach(Item in List) 
{ 
   DoAnotherThing(); 
} 
foreach(Item in List) 
{ 
   DoTheLastThing();
} 

The reason that the latter code which needs 2 more go-over-loops has a better performance, is because when it keeps calling DoSomething() thousands of times, some necessary variables are always warm in CPU registers. Very low costs are used to access those variables. On the other hand, if it calls DoAnotherThing() immediately after calling DoSomthing(), those variables of DoSomething() which already in CPU registers will cool down. Much more costs are needed to access these variables in the next loop.

Danny Chen