Opened 11 years ago

Closed 11 years ago

#1324 closed enhancement (done)

Improve performance of item collection views

Reported by: abeham Owned by: swagner
Priority: high Milestone: HeuristicLab 3.3.3
Component: Core.Views Version: 3.3.3
Keywords: Cc:

Description (last modified by swagner)

The ItemCollectionView and others are quite slow when the collection is cleared (the index references to the SmallImageList are updated for every removed item).

Change History (34)

comment:1 Changed 11 years ago by abeham

  • Status changed from new to accepted

comment:2 Changed 11 years ago by abeham

  • Owner changed from abeham to mkommend
  • Status changed from accepted to reviewing

r5057

  • Removed update of ImageIndex from RemoveListViewItem()
  • Added method RebuildImageList() for synchronizing the SmallImageList after a removal
  • Added a dictionary to provide faster access to items in the collection (similar to the optimization performed in the RunCollectionView in r4883)
Last edited 11 years ago by abeham (previous) (diff)

comment:3 Changed 11 years ago by mkommend

  • Owner changed from mkommend to abeham
  • Status changed from reviewing to assigned

review comments:

  • All events must be deregistered in the DeregisterContentEvents method. Escpecially ItemImageChanged and ToStringChanged are not deregistered and hence there are reference to the view as long as the content exists, regardless if the view is opened, closed or disposed.
  • The collection item events should only be registered once.

comment:4 Changed 11 years ago by abeham

  • Status changed from assigned to accepted

Thanks! It doesn't seem to be common knowledge that Content is not set to null when a view is closed. The old code probably expected that. I'll fix this for the other views also.

r5068

  • Fixed event (de)registration in ItemCollectionView

comment:5 Changed 11 years ago by abeham

  • Owner changed from abeham to mkommend
  • Status changed from accepted to reviewing

r5070

  • Fixed event deregistration in ItemArrayView and ItemListView

comment:6 Changed 11 years ago by mkommend

  • Owner changed from mkommend to swagner

comment:7 Changed 11 years ago by swagner

  • Status changed from reviewing to assigned

comment:8 Changed 11 years ago by swagner

  • Status changed from assigned to accepted

comment:9 Changed 11 years ago by swagner

Deregistration of the ItemImageChanged and ToStringChanged events of each item was already implemented in the control's Dispose method which is located in the designer file and therefore easily overlooked. For sure the better solution is to implement event deregistration in DeregisterContentEvents. I will removed the deregistration code in the Dispose methods.

comment:10 Changed 11 years ago by swagner

  • Description modified (diff)
  • Summary changed from Improve performance of ItemCollectionView to Improve performance of item collection views

comment:11 Changed 11 years ago by abeham

We both overlooked that. Yes, it seems better to do as much as possible in the user file.

comment:12 Changed 11 years ago by swagner

Changes in r5237:

  • Added performance optimization in all other item collection views
  • Checked and refactored item event registration/deregistration
  • Enabled null items in item collection views
  • Moved non-default Dispose methods from designer files into user files

comment:13 Changed 11 years ago by swagner

  • Owner changed from swagner to abeham
  • Status changed from accepted to reviewing

Please review changes of r5237 and then forward to mkommend for review.

comment:14 Changed 11 years ago by swagner

  • Owner changed from abeham to swagner
  • Status changed from reviewing to assigned

comment:15 Changed 11 years ago by swagner

  • Status changed from assigned to accepted

comment:16 Changed 11 years ago by swagner

Adapted method GetListViewItemsForItem of ItemListView and ItemArrayView to work with null items in r5238.

comment:17 Changed 11 years ago by swagner

Adapted method RebuildImageList to work with null items in r5239.

comment:18 Changed 11 years ago by swagner

  • Owner changed from swagner to abeham
  • Status changed from accepted to reviewing

Please review changes of r5237, r5238 and r5239 and then forward to mkommend for review.

comment:19 Changed 11 years ago by abeham

  • Owner changed from abeham to mkommend

Thanks for the changes, I reviewed them and have some minor comments

  • ItemArrayView
    • Since (Add|Insert|Remove|Update)ListViewItem and UpdateListViewItem(Image|Text) are protected they should throw an ArgumentNullException when listViewItem is null
    • This holds for the other views as well
  • ItemSetView
    • When moving an item in the ItemSetView the item is deleted from the set

I'll forward it to mkommend

comment:20 Changed 11 years ago by swagner

When item updates are still pending after the content of an ItemCollectionView has already been changed, the item is no longer held in the itemListViewItemMapping dictionary and consequently the update fails with a KeyNotFoundException. Fixed this in r5302.

comment:21 Changed 11 years ago by mkommend

  • Status changed from reviewing to assigned

comment:22 Changed 11 years ago by mkommend

  • Status changed from assigned to accepted

comment:23 Changed 11 years ago by mkommend

Added argument null checks in (Add|Insert|Remove|Update)ListViewItem and UpdateListViewItem(Image|Text) methods with r5371 as abeham suggested.

comment:24 follow-up: Changed 11 years ago by mkommend

  • Owner changed from mkommend to swagner
  • Status changed from accepted to reviewing

Reviewed the changesets r5237, r5238 and r5239.

I do not fully understand from abeham regarding moving of items in an ItemSetView and what the problem is if the item is deleted.

One thing which is still pending is the unification of the code of the different collection views (lots of duplicate code exists in the different collection view classes), but this should be addressed in a separate ticket.

comment:25 Changed 11 years ago by mkommend

  • Owner changed from swagner to mkommend
  • Status changed from reviewing to assigned

comment:26 Changed 11 years ago by mkommend

  • Status changed from assigned to accepted

comment:27 Changed 11 years ago by mkommend

Corrected ItemSetView to check for duplicates while dropping items with r5380.

comment:28 in reply to: ↑ 24 Changed 11 years ago by mkommend

Replying to mkommend:

I do not fully understand from abeham regarding moving of items in an ItemSetView and what the problem is if the item is deleted.

The problem is that the item is not readded when a move operations occurs. This can be tested in the OperatorGraphView which contains a OperatorSetView that is the only ItemSetView in the HL trunk.

comment:29 Changed 11 years ago by mkommend

  • Owner changed from mkommend to abeham
  • Status changed from accepted to reviewing

comment:30 Changed 11 years ago by abeham

  • Status changed from reviewing to readytorelease

Change looks good. I guess it's ready to release now

comment:31 Changed 11 years ago by swagner

  • Owner changed from abeham to swagner
  • Status changed from readytorelease to reviewing

comment:32 Changed 11 years ago by swagner

Created follow-up ticket #1407 for unifying the code of the different collection views.

comment:33 Changed 11 years ago by swagner

  • Status changed from reviewing to readytorelease

All additional changes look good. Thanks.

comment:34 Changed 11 years ago by mkommend

  • Resolution set to done
  • Status changed from readytorelease to closed
  • Version changed from 3.3.2 to 3.3.3
Note: See TracTickets for help on using tickets.