Performance: Refactoring generic List.RemoveAll... Speed lost in implementation and Array.Clear...

Well, I got railed after my previous posting on the List.RemoveAll method How is that RemoveAll method on the generic List actually implemented?. Apparently I'm not allowed to make basic comments anymore, and I quote "Now your remark is just information that just is not correct and of no interest for anybody"... Ouch man, that really hurts. Actually I could care less, but I don't want to scare off my "advanced" readers...

After looking at RemoveAll, there are two things we can fix. Apparently there is a nested while loop structure. Dan pointed this out after fixing up some code from .NET Reflector. The code would look something like the following:

while ( toffset < count ) {
    while ( toffset < count && match( data[toffset] ) ) {
        toffset++;
    }
    if ( toffset < count ) {
        data[hoffset++] = data[toffset++];
    }
}

If we follow this closely, we can see that some extra checks are being made. The first time in the outer loop we compare toffset < count twice... Every time this loop continues we do the same thing. This results in some extra comparisons. The next problem is that upon completing the inner loop we fall out and again compare toffset < count an extra and unncessary time. I can't account for every processor, so maybe there are some benefits to this method in certain JIT scenarios that I'm missing? If we refactor that loop we can get some minor performance improvements.

while ( toffset < count ) {
    if ( match( data[toffset] ) ) {
        toffset++;
    } else {
        data[hoffset++] = data[toffset++];
    }
}

The above loop removes the extra checks. This doesn't result in much of a performance improvement, but it is slight. We can jump to the bottom of the method and find another performance improvement. Right now there is a not-so-costly clear being performed. Defining not-so-costly is more difficult than you might think. We can actually get an extra 5-6% improvement in performance by removing it.

Array.Clear( data, hoffset, count - hoffset );
int length = count - hoffset;
count = hoffset;
version++;
return length;

That is what Reflector gives you for the end of the method. We create an extra stack local (length), and we perform the length subtraction twice... I honestly think doing it that way is faster than storing the length first and then reusing it (because of parallelism). I propose removing the Array.Clear and stack local.

count = hoffset;
version++;
return toffset - hoffset;

Little bit of food for though... The speed timings on RemoveAll vs RemoveAllFast are shown. I've made every attempt to isolate the actual method timings here, including pre-allocating the entire test structure, forcing a JIT of all of the methods outside of the loop timing, etc...

RemoveAllFast: 00:00:00.6309072, 0
RemoveAll: 00:00:00.7310512, 0

Published Friday, October 01, 2004 8:40 PM by Justin Rogers

Comments

Sunday, October 03, 2004 7:58 AM by Dan Golick

# re: Performance: Refactoring generic List.RemoveAll... Speed lost in implementation and Array.Clear...

I guess the risk we take with removing the Array.Clear is that some object won't get garbage collected because it is now still reachable.

I like the improvement in the while loop.
Do you think we could do better?
data[hoffset++] = data[toffset++];
seems awfully inefficient if there are multiple match failures in a row. If you modify the loop to find the start and count of the items we are keeping you could use Array.Copy (which is very efficient) to blast a block at one time.
Sunday, October 03, 2004 4:29 PM by Justin Rogers

# re: Performance: Refactoring generic List.RemoveAll... Speed lost in implementation and Array.Clear...

Ah, for reference type collections the clear makes good sense then, while value type collections it tends to make no sense whatsoever.

It would be an extra if check to add functionality for that (damn), which would cost you one extra operation on a reference type (making it a bit slower) and give you extra performance on value types (making them slightly more than a bit faster). Definite trade-off. Not to mention you'd have to add some type checks, probably during construction, to make the differentiation.

Array.Copy has a lot of extra overhead for making calls to it. There would probably be specific sets, where you are trying to remove ranges, that might make sense. If your data is so ordered, it would make much more sense to use an alternate means, such as RemoveRange, based on IndexOf and LastIndexOf expressions where you use your Predicate.

If you want to paste in an algorithm that uses Array.Copy instead, go ahead. I'll drop it into the test harness. Give me some data that is specifically stacked in it's favor as well.

Leave a Comment

(required) 
(required) 
(optional)
(required)