Removing items is slow

ajj3085 posted on Thursday, July 13, 2006

I have a DataGridView, and am using the following code to remove selected rows:

private void RemoveNumbersBtn_Click( object sender, EventArgs e ) {

       List<PhoneNumber> selectedRows;

       PhoneNumber num;

       selectedRows = new List<PhoneNumber>();

       foreach ( DataGridViewCell cell in

           MergeNumbersDataGridView.SelectedCells ) {

           num = (PhoneNumber)cell.OwningRow.DataBoundItem;

           if ( !selectedRows.Contains( num ) ) {

                selectedRows.Add( num );

           }

       }

       MergeNumbersDataGridView.SuspendLayout();

       foreach( PhoneNumber number in selectedRows ) {

           MergeNumbersBindingSource.Remove( number );

       }

       MergeNumbersDataGridView.ResumeLayout();

  }

It works, but seems like it could be quicker; it takes about half a second for the grid to update.  Any ideas?

Thanks
Andy

vargasbo replied on Thursday, July 13, 2006

Did you suspend event listners while you're doing the update?

xal replied on Thursday, July 13, 2006

Andy, I don't know if this is the cause of your problem, but you're using selected cells...

I'd loop through MergeNumbersDataGridView.SelectedRows and then get it's DataBoundItem.
That way you don't have to check whether your private list contains the item...


Andrés

ajj3085 replied on Thursday, July 13, 2006

Andrés,

Thanks for the tip, but the peformance hit seems to be the calls to Remove on the binding source, not looping the selected cells (there aren't many usually).

Andy

xal replied on Thursday, July 13, 2006

Andy,

Two questions:
-Why not calling the object directly instead of going through the binding source? you might want to try that to see if there's any difference... although there shouldn't be.

-Are you using the SortedBindingList? Currently the sorted binding list resorts the entire list whenever an item is removed(*) instead of just removing the removed item, which would be ideal... So, if you remove many items your SortedList will be resorting itself many times. Maybe if we had some sort of SuspendSort / ResumeSort in the SortedBindingList we could do a better job at handling this bulk change situations.



(*) The BindingList doesn't have a way for you to discover which item is being / has been removed. This is not true for added items since you can find out which item is being added through both of the binding list's events. Curiously, I recently contacted Rocky about this issue that affects SortedBindingList and prevents you from doing other stuff too... (that involve the need to know which items are being removed :D )

Andrés

tetranz replied on Thursday, July 13, 2006

Hi Andy

I've wrestled with the same slowness.

I've found that removing an item from a collection connected to a grid hits the UI with a lot of events. I worked around it with a somewhat kludgy shotgun approach. Whenever I do anything significant, including deleting from a collection, I try to disable as many events as I can.

Setting RaiseListChangeEvents on your collection will probably help. Also do this on the BindingSource.

I'm a bit short on time to really get into detail right now but I go one further and modify CSLA (BusinessBase I think) to give me a way of disabling INotifyPropertyChanged events on a business object. I might be getting  confused with CancelEdit() but I seem to remember that deleting from a collection caused the deleted object to raise INotifyPropertyChanged events.

I use the Janus grid so DataGridView might be different but Janus provide an event just before deleting and one after. I disable events before deleting and reenable after delete. I've found this to improve performance from the half second wading through maple syrup experience you're seeing to pretty well instant.

When I have time I really want to look into this a bit further.

Cheers
Ross

ajj3085 replied on Thursday, July 13, 2006

Ross,

Thanks, I was suspecting events hammering the UI as well.  I guess my gut instinct is to have a Remove( List<PhoneNumber> ) overload, which will fire just one event (Reset) after its finished removing all the specified items.

Any gotchas that anybody sees with this method?

Andy

ajj3085 replied on Thursday, July 13, 2006

Argh.

In the future I'll learn to check the visibility of the properties on the collection.  So the solution is exteremly simple; set RaiseListChangedEvents to false before removing, set it to true after, then call ResetBindings on the collection. 

Thanks everyone.

tetranz replied on Thursday, July 13, 2006

I'm pleased you solved it.

Just one thing that might not be worth wasting time on but FWIW ....

I suspect you discovered that after you set RaiseListChangedEvents to false, the item didn't disappear so you solved it with ResetBindings.

For things to be really working efficiently, you shouldn't have to do a ResetBindings. I think that sends a ListChanged event of type Reset to the grid which causes the grid to reload all data.

Normally when you delete an item, the collection fires a ListChanged event of type ItemDeleted and the grid should just remove that item. I derive my collections from my own base which derives from the CSLA class. In that base class I have a method that tells the collection to raise a ListChanged event of the type I specify. When deleting, I remember the collection index and then after the delete but before setting RaiseListChangedEvents true, I raise a single ListChanged event of type ItemDeleted. I know its kind of kludgy and I'm overriding the "natural" events but its the only way I've really been able to get my UI working efficiently without unnecessary events or reloading.

Cheers
Ross

ajj3085 replied on Thursday, July 13, 2006

Ross,

Thanks for the tip.  I chose ResetBindings though because my code may be removing many items at once, not just one.. so rather than raise ListChanged.ItemRemoved for each one, I opted to do a simple reset.

Its not a big deal, since each row is only four columns, and there typically will be fewer than five items in the list (which is why I was suprised that the grid took so long to update before my fix).

Andy