6

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?

mlorbetske
  • 5,529
  • 2
  • 28
  • 40
Jarlaxle2k5
  • 63
  • 5
  • 3
    Curious...does `lista.Items.RemoveAt(i)` differ in speed? Perhaps (counter-intuitively) the class has to go back and resolve the index itself. – DonBoitnott May 31 '13 at 15:56
  • 4
    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. – Reza Shirazian May 31 '13 at 16:13
  • 2
    @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. – Alexei Levenkov May 31 '13 at 16:20
  • DonBoitnott is right. RemoveAt(i) is the answer - verified. And yes, the logic in the loop doesn't make sense. – Yogee May 31 '13 at 16:40

6 Answers6

3

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.

David C
  • 3,610
  • 3
  • 21
  • 23
  • 1
    This is a good and compact implementation, although the AsParallel strikes me as overkill. – Reacher Gilt May 31 '13 at 16:23
  • The ToLower and Contains are both CPU costing operations, if the list is large it will benefit from the Parallel approach – David C May 31 '13 at 16:23
  • 2
    [`ListView.Items`](http://msdn.microsoft.com/en-us/library/system.windows.forms.listview.items.aspx) is a read-only property, so this doesn't work. – Michael Liu May 31 '13 at 16:28
  • 1
    You should replace the `.ToLower().Contains(Text)` with `.IndexOf(Text, StringComparison.CurrentCultureIgnoreCase) != -1`, which will avoid creating a new string for comparison. – Richard Deeming May 31 '13 at 16:29
  • 1
    @MichaelLiu wow, good call, i didn't even check if it had a setter. – David C May 31 '13 at 16:36
2

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.

Community
  • 1
  • 1
Asti
  • 12,447
  • 29
  • 38
  • 1
    [`ListViewItemCollection`](http://msdn.microsoft.com/en-us/library/system.windows.forms.listview.listviewitemcollection.aspx) doesn't have a `RemoveAll` method, so this doesn't work. – Michael Liu May 31 '13 at 16:43
  • @Yogee 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. – Asti Jun 03 '13 at 04:05
  • @Yogee SO can also be a place for general advice. Not just "give me teh codes". Nonetheless, I've edited it to weed out ambiguity. – Asti Jun 03 '13 at 04:07
  • 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. – Yogee Jun 03 '13 at 07:13
2
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).

cramopy
  • 3,459
  • 6
  • 28
  • 42
Yogee
  • 1,412
  • 14
  • 22
  • 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. – Jarlaxle2k5 Jun 03 '13 at 10:39
  • @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. – Yogee Jun 03 '13 at 12:14
  • @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]? – Yogee Jun 03 '13 at 12:20
  • 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. – Jarlaxle2k5 Jun 03 '13 at 13:13
  • 1
    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 !!! – Jarlaxle2k5 Jun 03 '13 at 14:07
  • 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. – Yogee Jun 03 '13 at 15:00
  • 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 !!! :) – Jarlaxle2k5 Jun 04 '13 at 07:22
0

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.

0

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--;

user3739815
  • 1
0

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.

open-collar
  • 1,404
  • 1
  • 16
  • 22