[ASCollectionView / ASTableView] Optimize reloadData and reloadSection: methods.

[lkzhao]

Calls reloadData & reloadSection on UITableView/UICollectionView instead of deleting/inserting one by one.
Similar to #1093.

Please review this but do not merge yet. still seeing that bug where collectionView will cause inconsistent crash when inserting rows unless [collectionView numberOfSections] is called before inserting.
@nguyenhuy
@appleguy

[appleguy]

@lkzhao this is great, very glad to see this! Are there any areas in particular that I should focus on for review? @nguyenhuy 's original disk to attempt something similar was reverted a couple times, but I can't remember if we ever really landed it after the second time… this this is a attempting to do something similar with a new approach, correct? Just want to make sure I understand where the prior one left off, it will not re-land again?

[lkzhao]

@appleguy Yes, both are optimizing reload calls. This patch includes the other one that Huy made and fixed the crashes we had earlier. I actually took some code directly from his fork. We can land this instead.

Few notes about what I did here:

1. Removed _deleteSection, _insertSection etc. These don't do much and made it very confusing to read the code.
2. (Please review this part) Every node editing methods previously all have a completion callback scheduled on the main thread and update the _completedNodes on the main thread. This made it hard and cause crashes when doing batch operations like ReloadSection/ReloadData etc.
   Now these methods only change _editingNodes and do nothing with the _completedNodes. After we are done with the operations, we call commitChangesToNodesOfKind and provide a callback there.
3. Optimized reloadSection/reloadData/reloadItems/deleteSection/insertSections on TableView and CollectionView. Previously, all of these actions result in deleteItems/insertItems calls to TableView and CollectionView. Now it makes one specific call. (for example, reloadSection calls tableview reloadSections: withRowAnimation instead of making deleteRowsAtIndexPaths: and insertRowsAtIndexPaths:)

[appleguy]

Thank you Luke, this summary was extremely helpful for me. I will review in detail soon.

Sent from my iPhone

On Feb 6, 2016, at 10:06 AM, Luke Zhao notifications@github.com wrote:

@appleguy Yes, both are optimizing reload calls. This patch includes the other one that Huy made and fixed the crashes we had earlier. I actually took some code directly from his fork. We can land this instead.

Few notes about what I did here:

Removed _deleteSection, _insertSection etc. These don't do much and made it very confusing to read the code.

(Please review this part) Every node editing methods previously all have a completion callback scheduled on the main thread and update the _completedNodes on the main thread. This made it hard and cause crashes when doing batch operations like ReloadSection/ReloadData etc.
Now these methods only change _editingNodes and do nothing with the _completedNodes. After we are done with the operations, we call commitChangesToNodesOfKind and provide a callback there.

Optimized reloadSection/reloadData/reloadItems/deleteSection/insertSections on TableView and CollectionView. Previously, all of these actions result in deleteItems/insertItems calls to TableView and CollectionView. Now it makes one specific call. (for example, reloadSection calls tableview reloadSections: withRowAnimation instead of making deleteRowsAtIndexPaths: and insertRowsAtIndexPaths:)

—
Reply to this email directly or view it on GitHub.

[nguyenhuy]

Do we need to notify the _layoutFacilitator here? cc @binl

[binl]

yes, we will need to do

[_layoutFacilitator collectionViewWillEditCellsAtIndexPaths:@[[NSIndexPath indexPathForItem:0 section:0]] batched:YES/NO];

[nguyenhuy]

I have 2 main concerns/feedbacks about this PR:

1. We should clearly document a few things for each of the methods in ASDataController+Subclasses.h:
   • It should (and is assuming to) be called on _editingTransactionQueue.
   • It blocks the calling (background) thread.
   • It only modifies _editingNodes.
   • Its caller is expected to commit the changes to _completedNodes on main afterward.
2. I think adding commitChangesToNodesOfKind is a really good idea that helps to simplify the controller. However, deep copying a 2D array is expensive and should be avoided. That's why in the current implementation, we don't deep copy after deletes, but instead directly remove items. Can we do the same thing here?

Other than that, I reckon this new implementation should be faster than the one on master because we update _completedNodes and notify delegate once after all operations finished 👍

[lkzhao]

@nguyenhuy Thanks for the review & feedbacks!! There are some code that I copied/converted and I am not entirely sure what they do. Let me talk to you tomorrow about some of the details int your review.

[appleguy]

Very excited to land this! Thank you @lkzhao