perf(virtual-list): relaxed restrictions for fast path

[manucorporat]

Short description of what this resolves:

Changes proposed in this pull request:

Ionic Version: 1.x / 2.x / 3.x

Fixes: #

[masimplo]

This does not change the item reference and thus is not expected to generate a diff.

    this.items[index] = { value: Math.floor(Math.random() * 10000), someMethod: () => '!!' };

is more appropriate.

[masimplo]

The if here will only filter out adds after last record.
I tried something like:

        // add new record after current position
        if (item.previousIndex === null && cindex > lastRecord) {
          console.debug('adding record after current position, no recalculation needed');
          return;
        }

        // remove record after current position
        if (item.previousIndex > lastRecord && cindex == null) {
          console.debug('removing record after current position, no recalculation needed');
          return;
        }

        needClean = true;

to add more explicit checks for filtered out cases (we can probably consolidate them later on)
This now allows 3 cases to not trigger cleaning (the first two obvious, the third not so much).

1. Adding a record after lastRecord
2. Removing a record after lastRecord
3. Changing a record after lastRecord.

When changing the reference of a record, changes is populated with two entries, one for the removed record and on for the added record. So forEachOperation will be triggered twice, with half the info each time.

logs after triggering a random record changed that happened on position 179:

virtual-scroll.ts:451 virtual-scroll, prevIndex: 179 cindex: null lastRecord: 33
virtual-scroll.ts:460 removing record after current position, no recalculation needed
virtual-scroll.ts:451 virtual-scroll, prevIndex: null cindex: 179 lastRecord: 33
virtual-scroll.ts:454 adding record after current position, no recalculation needed

[masimplo]

LastRecord is modified with scrolling and reset only after needChanges is set to true at least once, so after scrolling all the way to the bottom all add/deletes/updates (except appends) will take slow path until a complete recalculation takes place, even if in the mean time I have scrolled all the way up to the bottom.
Not sure if there is a better approach on this, just pointing out the current state.

[masimplo]

I have also noticed that setting virtualTrackBy has no effect, as when the virtualScroll setter is called, this.virtualTrackBy has not taken a value yet. Triggering the setter again (by setting a new array reference) sets the virtualTrackBy successfully.

[manucorporat]

@masimplo feel free to clone this PR and submit a new one, unfortunately I have to work in some keyboard issues during this cycle, but I will re-evaluate the virtual-list performance in the next cycle.

Another improvement we could make, in slow path we don't need to purge the array of nodes, since we could reuse them. This way we would save the need to recreate all the embedded views each time the slow path is taken.

[masimplo]

It might just be getting late for me, but shouldn't needClean = true be outside/after these two ifs since it should happen in all other cases?

[masimplo]

Read your code again with a clear head and I was mistaken about the earlier comments. The comments confused me, because they state the exactly opposite of what you are actually doing.

// add new record **AFTER** current position
if (pindex === null && (cindex < lastRecord)) {
   console.debug('adding record **BEFORE** current position, slow path');

I guess I should know better. Code never lies, comments... sometimes. 👨‍💻 Will be now closing #11491 in favour of this one.

Thanks for the effort Manu, really appreciate you taking the time and dealing with this on the side.

If you ever get virtual scroll into your crosshairs again it would be nice if you could find a way for updates to not cause destroying of the rendered elements at all as you mentioned the other day

Another improvement we could make, in slow path we don't need to purge the array of nodes, since we could reuse them. This way we would save the need to recreate all the embedded views each time the slow path is taken.