VirtualScroll does a complete reset when ANY item has changed.

[masimplo]

Ionic version: (check one with "x")
[ ] 1.x
[ ] 2.x
[x] 3.x

I'm submitting a ... (check one with "x")
[x] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or http://ionicworldwide.herokuapp.com/

Current behavior:
I have a list with 1000 items that is bound to a virtual scroll component. If I replace any of 1000 items with a new reference (i.e. items[500] = new Item();) then all components rendered inside the virtual scroll are destroyed and recreated.

Expected behavior:
The items should not be re-created especially if the affected item is not in the list of rendered items.

Steps to reproduce:
http://plnkr.co/edit/WljhJIp8ZDqAMshpCQPr?p=preview Please look at the console and notice that after 5 secs timeout all nodes are destroyed and re-created although a single item has changed.

Related code:
After some debugging I found that the decision of whether rendered items will be re-created is taken around line 423. If I understand correctly the only occasion when the rendered items will not be re-created, if any changes are present, is when you add an item at the end of the list, which does not make much sense.

if (changes) {
      changes.forEachOperation((item, _, cindex) => {
        if (item.previousIndex != null || (cindex < this._recordSize)) {
          needClean = true;
        }
      });
    } else {
      needClean = true;
    }

I believe a better, but still not over-engineered implementation would be to set needClean only if an item matching a rendered item has changed.
@manucorporat I would love to hear you point of view here, in case I have understood something completely wrong.

       if (changes) {
            var recordIndicesInView = this._nodes.map(n => this._cells[n.cell].record);
            changes.forEachOperation(function (item, _, cindex) {
                if(needClean) return; // no need to check further after at least a single match has been found
                if(recordIndicesInView.some(i=> item.previousIndex === i || cindex === i)) {
                    needClean = true;
                }
            });
        }
        else {
            needClean = true;
        }

Ionic info: (run ionic info from a terminal/cmd prompt and paste output below):

Cordova CLI: 6.5.0
Ionic Framework Version: 3.0.1
Ionic CLI Version: 2.2.2
Ionic App Lib Version: 2.2.1
Ionic App Scripts Version: 1.3.2
ios-deploy version: 1.9.0
ios-sim version: 5.0.8
OS: macOS Sierra
Node Version: v6.10.0
Xcode version: Xcode 8.3.1 Build version 8E1000a

[manucorporat]

@masimplo yes, adding items at the tail is a cheap operation, I added this fast path not a long time ago.

I believe a better, but still not over-engineered implementation would be to set needClean only if an item matching a rendered item has changed.

I don't think that's true. let's say you remove the second item, and we are rendering from 50 to 100, we have to re update everything, since now we should be rendering from 49 to 99.

It can be done, but feels like a very complicated problem. Each item can have a different size.

But please, feel free to try improve the needsClean algorithm, I will be happy to assist you.

[manucorporat]

mmmm maybe we can add another fast path if the change is not an addition or a removal? is that what you meant?

[masimplo]

I see. But if you are rendering the first 20 items and have a list of 1000 items though and a change happens at item say 500, I believe it is wasteful to recalculate everything, since nothing really changes for the VS.
Also for items before and after the rendered list, an update should not affect the calculation of VS as well.
Just these two would provide a huge improvement, especially in very large lists.

For Add/Delete to items before our current position, I will have to give it some thought, but I believe a more "surgical" approach might be in order (like finding the offending cell and adding/removing its height to top offsets or something similar, again I need to think about this before coming with a concrete solution).

Side story on how I got into this
In my case I am binding an array of immutable items and every update in any data is a reference change for that item and thus every single update to any field in any of the 1000s of items triggers a recalculation of everything so VS is pretty unusable, especially since I am using custom components that are destroyed and recreated, in every such trigger and not simple ion-items.

[manucorporat]

But if you are rendering the first 20 items and have a list of 1000 items though and a change happens at item say 500, I believe it is wasteful to recalculate everything, since nothing really changes for the VS.

I do agree, I am working in a PR

[manucorporat]

But I think it does affect adding/removing before our current position...

[manucorporat]

Can you play with #11297 ?

[masimplo]

Yeap, will keep you posted.

[masimplo]

I have added some comments to your PR.

Summarising

Current state is:

1. Append always takes fast path.
2. Add random takes fast path if in position after lastRecord.
3. LastRecord is modified with scrolling and reset only after needChanges is set to true at least once, so after scrolling all adds will take slow path until a complete recalculate takes place.
4. Change random always takes slow path as we get two records in changes. One case prevIndex thus triggering slow path always.
5. Delete always takes slow path
6. Also VirtualTrackBy is not set on time, thus not taking part in all above calculation correctly.

With replacing the if statement inside the forEachOperation with the two proposed if statements 1 and 2 still stand, but 4 and 5 also take fast path if after lastRecord.
If lastRecord can be recalculated while scrolling up (or be calculated based on values that change when scrolling up) we could get away with one scenario triggering an additional cleanup.

I am now thinking about what should happen when an update happens in any position and not just after lastRecord. Do you think that a cleanup could be avoided, or is it that even an update could change the height of a record? And if so, could there be an optimisation where the user could specify that elements will have a fixed height and improve performance throughout operations?

Take a look and let me know.

[masimplo]

@manucorporat I Just created pull request #11491 with what was discussed above.

I also have figured out why VirtualTrackBy is not working and have a solution using a setter and re-initializing _differ. I have not included this in the above PR to keep things clean. I will open a separate PR for that.

[masimplo]

This has been greatly improved by merging #11297

[ionitron-bot]

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.