[grid] arrayDataProvider renders cells multiple times when items changed

[asyncLiz]

Description

The grid renders cells multiple times when items are first set, even though the model does not change. The more complex the cells and the greater the number of columns, the worse the slowdown.

Expected outcome

A cell should only be re-rendered when the row model changes.

Actual outcome

Cells are rendered multiple times when items are set.

Unlike vaadin/vaadin-grid#1264, this isn't an issue with the observers so much as it's an issue with properties. When the items change, this function is called:

    _itemsChanged(items, splices, isAttached) {
      if (!isAttached) {
        return;
      }
      if (!Array.isArray(items)) {
        if (items === undefined || items === null) {
          this.size = 0;
        }
        if (this.dataProvider === this._arrayDataProvider) {
          this.dataProvider = undefined;
        }
        return;
      }
      this.size = items.length; // 4 renders (3 sync, 1 async)
      this.dataProvider = this.dataProvider || this._arrayDataProvider;
      this.clearCache(); // 1 render
      this._ensureFirstPageLoaded();
    }

Changing the size updates the _effectiveSize which causes the scroller to rescroll (even if not needed because the first visible index is 0 and it's already scrolled to the top), which causes the iron-list to

1. flush and render synchronously, and then
2. assign models

After that, the scroller itself will

3. synchronously re-render by assigning models and
4. asynchronously re-render by updating items

Finally, after the size has been set, _itemsChanged will

5. clear the cache which will assign models and cause the 5th re-render.

Solution?

I imagine that most of the properties and effects that cause a re-render could happen independently of the items changing and truly require a re-render. The grid doesn't know and it's being safe.

However, what the grid could know is whether or not a row's model has changed. If the item (and all of its properties) are the same, and the index/level/expanded/selected/detailsOpened properties have not changed, it's safe to assume that rendering the row again will result in the exact same DOM structure.

    _updateItem(row, item) {
      row._item = item;
      const model = this.__getRowModel(row);

      // Check to see if the previous model for the row equals the current model.
      // Since __getRowModel() returns a new instance, this would need to be a deep
      // equality check. This would also need to be a deep check if the item
      // is an object, since Polymer lets you notify the path of an item changing and
      // that would require a re-render.
      // For demo purposes though:
      if (
        row._prevModel && 
        row._prevModel.index === model.index && 
        row._prevModel.item === model.item &&
        row._prevModel.level === model.level && 
        row._prevModel.expanded === model.expanded && 
        row._prevModel.selected === model.selected && 
        row._prevModel.detailsOpened === model.detailsOpened
      ) {
        // The model has not changed for this row, don't re-render it
        return;
      }

      row._prevModel = model;
      /* ... render the row */
    }

Patching _updateItem() may also promote gains from other properties or data providers that may be updating the grid and cause it to think it needs to re-render a row when it really doesn't.

Live Demo

https://github.com/asyncLiz/vaadin-grid-perf

Steps to reproduce

1. Clone https://github.com/asyncLiz/vaadin-grid-perf
2. npm start
3. Click "Load 500" button. The renderer function will count the number of times the first row's cell is rendered (4-5 times, it seems to vary, I'm assuming due to async timing variations).

Potential solution:

1. Load 500 items (should see 4-5 renders)
2. Clear items
3. Patch grid
4. Load 500 items (should see 1 render)

Browsers Affected

• [x] Chrome
• [x] Firefox
• [x] Safari
• [x] Edge
• [x] IE 11
• [x] iOS Safari
• [x] Android Chrome

[tomivirkki]

Thanks for the report. Not sure if the solution proposal is valid since it checks the item equality with row._prevModel.item === model.item which could evaluate to true even if a sub-property of the item has changed. In that case a re-render would still be expected.

Example: The first demo (https://cdn.vaadin.com/vaadin-grid/5.2.5/demo/#grid-basic-demos) should update the "First Name" of the first item with the following:

const grid = $0;
grid.items[0].name.first = 'Foo';
grid.clearCache();

Edit: I see you already brought this up at ("..and all of its properties.."). Could be difficult to deep scan trough the diffs of the entire item object.

[asyncLiz]

You could also provide some property or callback for the user to determine whether or not two items are equal.

html`
  <vaadin-grid 
    .items="${items}"
    .areItemsEqual="${(a, b) => a.id === b.id}"    
  >
  </vaadin-grid>
`

That would prevent vaadin-grid from having to include a deep equality checker. You could even make this re-rendering logic optional and only enabled if the user provides a function to determine item equality.