More on PerfProductivity: The devil is in the details…

I enjoyed reading (and forwarding) the comments on my recent post on the productivity performance tradeoff… Many of you (rightly so) pointed out that the details really mater in such a discussion. So I picked one that we are struggling with right now.And I think it is generally interesting to see how enumerator works in List<T>…

Productivity:

List<T> maintains the one of the same invariants that ArrayList offers: You are not allowed to modify the list while it is being enumerated.We do this in order to cheaply provide a level of error checking. It is too expensive for List<T> to be resilient to such changes so we do just enough runtime checks to ensure the invariant is enforced. The common scenario this helps with is:

List<string> l = newList<string>();

l.Add("Joel");

l.Add("KCwalina");

l.Add("Kit");

l.Add("Peter");

l.Add("Joel");

l.Add("Ahmed");

l.Add("Joe");

foreach (string name in l)

{

if (name.Contains("K"))

{

l.Remove(name); //This is not allowed

}

}

With the checks you get an InvalidOperationException in the loop iteration right after Remove is called, without the checks you would get unpredictable results based on if you have enumerated passed the element or not.These are very subtle bugs to find.If you have ever gotten this exception you can appreciate that it is easy to get into this case without meaning to.As a test I took out the check that enforces this invariant and I was surprised that I got a NullReferenceException on the 7th (last) iteration through the loop on the if-branch. Any idea why?I for one would find this a subtle bug to go find-and-fix.

Performance:

Of course, this check comes at a cost.To implement this invariant we use a “version stamp” on each instance of List<T>… Every time you modify the list we increment the version stamp. When you get the enumerator (or the foreach statement does it on our behalf) the enumerator stores that version stamp away. Then each time you (or the foreach statement on your behalf) calls MoveNext() the snapped version stamp is checked against the current version stored in the list being enumerated. If they are different we throw.Here is the code for MoveNext() from the List<T> enumerator:

publicbool MoveNext()

{

if (version != list._version) thrownewInvalidOperationException();

if (index == endIndex)

returnfalse;

index++;

returntrue;

}

That is an extra if-branch each time through the loop. For a small number of items the overhead of setting up the loop dwarfs this cost, but for large list this check does show up, although I have never seen it show up top on the list in a real-world scenario.

Feedback

So, what do you think?Does the benefit of checking the invariant at runtime outweigh its cost? If we were able to do this check only if you are running with the debugger enabled in VS would that be good-enough or is there value in having this check always on?

I believe the beneift of checking outweighs the cost in this scenario for two reasons. First, no matter how well you write the documentation there will be a signifigant percentage of people who do not realize this invariant exists. Secondly, I’ll learn pretty quickly about this invariant if I write a foreach statement like the one in your first example. I’ve learned it, and I’ll never try to modify the collection inside a foreach statement again. Then I make my program multi-threaded, and once in a blue moon a thread does modify the collection while a second thread is iterating. In this case the check could save a large amount of debugging effort.

I believe it has to be consistent in debug and release builds. Turning off asserts for C++ production code always made me uneasy. Yes, the error message meant nothing to the end user, but at least the program didn’t throw up a screen full of numbers which looked normal but meant nothing.

A design philosophy that has worked for me in the past is : if a design decision fork has two equally good branches, it’s probably better to keep the decision open and allow the final user to decide. (when possible !! )

In this example, I’d take the check out of the loop and expose a property – HasChanged – which will allow the developer to explicitly check within the loop, if the object has changed since the enumeration started.

This works as long as developer productivity doesn’t become a synonym for developer ignorance – allowing developers to get away with not knowing the behaviour of the objects they are using.

I believe in this case legacy reigns. You can’t modify the behavior to remove the checking because it already exists in other API’s and should stay there. Now, you mentioned getting a null reference exception, which is obvious in your scenario, because the endindex item is getting cached because you are assuming the length of the list isn’t going to change. Even worse, if you did a Remove and a TrimToSize, you’d eventually get an OOB exception.

Now, that said, an invariant enumerator would have to not cache endindex and instead use the .Count property (or most liekly the private count field) each time through MoveNext. Else you can’t safely enumerate even while changes are occuring. Is changing the enumerator to use .Count intead of an endindex going to slow it down sufficiently that we should just be checking the version anyway?

I’d say make an GetUncheckedEnumerator method for power users, but that won’t get enabled through C#’s foreach statement. As that is the case, it is worthless. If I want enumeration of a List<T> I’ll just use a for loop. Performance means for loop, Productivity means foreach. So I’m not seeing the trade-off anymore, only a choice.

I’m not an expert in .NET, but isn’t it possible to specify the code to generate by using special attributes for the variable declaration. Would this be too hard and impractical to implement? Or .NET attributes were not meant to be used this way to customize compiler output? Or it’s generally bad to allow developers choose to skip checking in order to get faster code?

In any case I would think turning check off in release builds could work, but then most likely you would have to find a good way to notify all developers when such change in the output code takes place. Of course documentation will outline this "feature", but I would not be very surprised if not every single developer examined the documentation thoroughly. And people tend to forget anyway. And since this could potentially change the expected behavior of the application people will have to be aware of this taking place. So imho the biggest problem would be to get the word to the masses. And of course there is the multithreaded problem described earlier.

Default the optimization to on perhaps, but expost it (and others) as compiler switches. In this case, for those that want that extra speed and know they are using the List correctly (or are willing to deal with the ramifications), then let them.

For most situations leaving the check in would provide the most benefit, especially since the 1st test run that exercised the .Remove call would throw an exception, providing instant feedback during a test of a bug. Taking out the check means that testing may not even find this if the test data does not include all the edge cases.

Since taking out the check results in a misleading NullReferenceException this is even more reason to leave it in. When the foreach accesses the list with a bad index (beyond its end) I would have expected an IndexOutOfRangeException. Instead, it must be casting the result to a null string reference. How is the Current property implemented? Misleading information is almost as bad as no information.

I agree with Justin – if I want high performance I’ll use a for loop where I explicitly take on the responsibility of handling the list correctly.

To explain why you get the NullRef instead of the IndexOutOfRangeException, you have to understand there is a larger underlying array in this example. Say it is 20 elements. As the enumerator goes through the list, it just increments an offset. Once you get to element 7, and one of your elements is already gone, it will return whatever is in the underlying array, which in this case is null. This is a very insidious issue because the enumerator uses a fixed end-index instead of a count.

Now, you might ask, why does it happen on element 7? Well, we started with 7 elements and so endindex is giong to be 7 (6 actually). Then we removed element KCwalina, and the entire array underneath gets shifted down by 1. That puts Kit at the current offset, meaning we skip this element entirely. The next MoveNext will increment us one and put us on Peter.

Brad actually got to demonstrate two bugs with a single sample. One bug is the array element skipping If I were using a for loop and I removed an element based on index, I would decement my index pointer, so the next time through the loop, I was placed at the same index and could process the new element. You might try to be savvy and just bump to the top of the for…loop, but this offers up the problem of whether or not this was already the last element in the list so there wouldn’t be any more elements to process (a third type of issue).

The second bug is walking past the end of the list, but still living within the bounds of the underlying array, in which case you’ll get null’s for object types, and even worse, 0’s for integer types, and 0.0 for float types. You can see where that would really suck, because you wouldn’t even know what you had done, since you won’t get a NullRef and you won’t get any bounding exceptions either.

1. Use foreach(…). I’m relying on a shortcut to produce easy code without having to take control of all the details.

2. Use for(…). I’m taking control of the details, and I know enough about how the class (List<t> in this case) works to enumerate it myself. I’m doing that because I understand the issues, and need the benefits that all this extra work will get me.

It seems to me that developers in case one are clearly saying "productivity for me, please", and are willing to take a small perf hit. In case #2 the developer is saying "I know the risks, give me control", and can address thier own perf issues.

I’d be more interested in an example you have where we *can’t* have it both ways. I think the discussion may be a bit more heated. Personally, when choosing between perf and productivity, I want both.. 🙂

I agree essentially with Justin. We created some base collection classes in VG.net and had to make the exact same decision. We decided to remove the version number, not because it makes the loop slower, but because it makes every instance of the collection larger. This is a more important factor. That version number is not retaining useful information most of the time, and is wasteful. Why force everyone to store it?

As Justin points out, you can eliminate any possible exceptions by making the enumerator more robust, checking to see if the Count changed. This does not prevent changes to the list, but actually adds flexibility — limited changes might be allowed. If people really want to change the list and iterate at the same time, they can always use a copy, or for (int i = 0; i <….).

So put error checking in the enumerator, and let users use the "for" to get max speed.

This argument applies to an ArrayList type of class, not a true linked list.

Let’s see… if we extend the performance argument, I should take out all the error checking in my code and all will be good and lightning fast as well. Sounds good to me; no more writing those pesky if statements.

I just don’t buy it. For our customers, stability and security go hand-in-glove. Without the error checking, you miss the obvious logic error (which is based on implementation details). Would the error be found in testing? Maybe – depends on the tests performed. Would the error be found in the field? Definitely. That’s not a good place or time for discovering nuances about the underying library.

If statements comparing values in memory for error checking are essentually free. Thank the hard working engineers at Intel for their instruction pipeline optimizations. Please sprinkle on liberally.

Unless you can formalize what it means to change an underlying collection while iterating over it, then you should probably throw as an unsupported operation.

For example, if I am iterating over a tree then I expect to get every element in the tree. If someone removes an element which causes a rebalance then it’s almost certainly gauranteed that the iterator will be in a hosed position at that point and that continuing the iteration will not give me all the correc telements (it might return an old element again, or it may miss elements). Because of this it’s better to throw and make the consumer realize that they are almost definitely not doing the right thing.

If you need to expose mutability semantics that work in the presense of iteration, then it needs to be something that works hand-in-hand with the iterator. For that reason I see those operations as being better suited on the iterator. If you want to add an element, tell the iterator. This will ensure that after the operation the iterator and underlying collection will be in a sane state.