c# ListView.Items[i].remove is very slow

Question

It is my first time here and I am struggling to solve this issue. I have this piece of code:

try
{
    progressBar1.Maximum = lista.Items.Count;
    lista.BeginUpdate();

    for (int i = 0; lista.Items.Count > i; i++)

    //for (int i = lista.Items.Count - 1; -1 < i; i--)
    {
        if (lista.Items[i].SubItems[1].Text.ToLower().Contains(Text) == false)
        {                        
            lista.Items[i].Remove();                        
        }

        progressBar1.Value = progressBar1.Value + 1;
    }

    lista.EndUpdate();

    progressBar1.Value = 0;
}
catch (Exception errore)
{
    txt_info.Text = "" + errore.Message;
    progressBar1.Value = 0;
}

The method lista.items[i].remove is extremely slow. lista is a ListView and I am working on a log file bigger than 50,000 lines. Is there anyway to speed up the process?

Heeley

Curious...does lista.Items.RemoveAt(i) differ in speed? Perhaps (counter-intuitively) the class has to go back and resolve the index itself. – Apothegm

You shouldn't change the size of a data structure (remove items from it) within a For loop. Try re-writing the loop without having the remove in it. For example flag the indexes that need to be removed in the for loop and then remove them outside of it. – Hainan

@RezaShirazian One clearly can't do that with foreach... I believe it reasonably ok to use for to remove items (eventually some piece of code have to do it anyway) - also obviously incorrectly written (like in sample above due to skipping items next to one that just removed) is bad idea. – Circus

DonBoitnott is right. RemoveAt(i) is the answer - verified. And yes, the logic in the loop doesn't make sense. – Kava

Answer 1

ListViewItem[] allElements = new ListViewItem[listView1.Items.Count];
listView1.Items.CopyTo(allElements, 0);
List < ListViewItem > list = allElements.ToList();
list.RemoveAll(item => item.SubItems[1].Text.ToLower().Contains(TextToFind) == false);
listView1.BeginUpdate();
listView1.Clear();
listView1.Items.AddRange(list.ToArray());
listView1.EndUpdate();

First Rule is never update list in for loop. Your logic will only run till half of the list. I guess that's not what you want.
I've seen that manipulating listview.items is very slow even after using BeginUpdate and EndUpdate. Key is to do the manipulation outside (in list or so) and then re populate the list with AddRange (which is much faster than Add).

Kava

i have tried the code above but gives me back an empty list. Do i miss something ? In your comment you said that you are using foreach loop but i don't see it in the code that you left. – Heeley

@user2441083: My older comment was using foreach and then I was removing elements from list. But then I used RemoveAll which didn't require foreach. Btw, when does the list becomes empty? After RemoveAll? if so, you need to check if the condition written in "RemoveAll" might be doing so. – Kava

@user2441083: I wrote the same logic in my application and it is working fine and fast enough.. takes around 2450 milliseconds for removing 10% of list times for 50,000 entries of two dimensional string as LIstViewItems in listView1.Items. Are you sure you want to check for item.SubItems[1] and not item.SubItems[0]? – Kava

I did a bit of debug and the problem is actually after the lista.clear(). It seems that the AddRange is not filling the list back. – Heeley

done !!! It works perfectly, i have just readded the column lista.Columns.Add("Description"); before use the AddRange. Thank you so much for your kind support !!! – Heeley

gr8. I think you will have to remove the progress bar now. You can put something like "Loading..." message. If my answer was 'the answer' please mark it as answer. – Kava

yeah no need for the progress bar now and yes yours is the correct answer :) the speed gain is off the wall !!! Thank you so much !!! :) – Heeley

Answer 2

I would take a different approach and use LINQ, something like this :

lista.Items = lista.Items.Where(x=>x.SubItems[1].Text.ToLower.Contains(Text)).AsParallel().ToList();

Basically, rebuilding the list once rather than trying to remove individual items over and over again.

Polysyllabic

This is a good and compact implementation, although the AsParallel strikes me as overkill. – Malefactor

The ToLower and Contains are both CPU costing operations, if the list is large it will benefit from the Parallel approach – Polysyllabic

ListView.Items is a read-only property, so this doesn't work. – Topmast

You should replace the .ToLower().Contains(Text) with .IndexOf(Text, StringComparison.CurrentCultureIgnoreCase) != -1, which will avoid creating a new string for comparison. – Begone

@MichaelLiu wow, good call, i didn't even check if it had a setter. – Polysyllabic

Answer 3

The simplest option would be to use the list's own RemoveAll method.

list.RemoveAll(x => !x.SubItems[1].Text.ToLower().Contains(Text))

P.S.

You might want to look for speed gains in the actual comparison. Using String.Compare is much faster if your requirement fits it. If you want to check for a sub-string, I would suggest using ToUpperInvariant for invariance related matters - it's designed to be faster.

Succory

ListViewItemCollection doesn't have a RemoveAll method, so this doesn't work. – Topmast

@Kava I was suggesting that filtering is several times faster for direct comparison - not a replacement for Contains. If he wants to use Contains, then ToUpperInvariant is faster. – Succory

@Kava SO can also be a place for general advice. Not just "give me teh codes". Nonetheless, I've edited it to weed out ambiguity. – Succory

Thanks Asti for editing the answer. I will remove my obsolete comment. Btw, the code that I posted in this thread is reasonably faster. I am not sure if it will make any difference in performance as I see my list.RemoveAll is fast for 50Krecords. Bottleneck in this case is directly manipulating (like edits/deletes) the listview items. I noticed that if you clear and add elements (especially using AddRange), it is much faster. – Kava

Answer 4

You could stick it in a background worker and have it do this on it's own. Therefore your users could still use the program while this process is occuring.

Mcdavid

Answer 5

If you remove an item in for loop, you should set the counter to 1 less so you won't miss one. Because you remove [i], [old i+1] becomes the [new i] (Items.Count decreases 1 also) and you will miss checking the [new i].

e.g: ListView.Items.Remove[i]; i--;

Popular

Answer 6

Lists are slow to access dynamically and much better suited to iteration. I would suggest manipulating the data outside of the listview (maybe by copying the rows you want to display into a temporary list one at a time as you iterate across the source collection) and then assigning to the listview at the end. This can be done on a different thread (to improve UI performance) and doesn't require expensive lookups.

Alanaalanah