From b40840b969a217ff6f7c758f08bce4d7e5d0edd6 Mon Sep 17 00:00:00 2001
From: Kevin Schaaf <kschaaf@google.com>
Date: Thu, 20 Feb 2020 23:26:59 -0800
Subject: [PATCH] Fixes for several related dom-repeat chunking issues. Fixes
 #5631. * Only restart chunking (resetting the list to the initialCount) if
 the `items` array itself changed (and not splices to the array), to match
 Polymer 1 behavior. * Add `reuseChunkedInstances` option to allow reusing
 instances even when `items` changes; this is likely the more common optimal
 case when using immutable data, but making it optional for backward
 compatibility. * Only measure render time and throttle the chunk size if we
 rendered a full chunk of new items. Ensures that fast re-renders of existing
 items don't cause the chunk size to scale up dramatically, subsequently
 causing too many new items to be created in one chunk. * Increase the limit
 by the chunk size as part of any render if there are new items to render,
 rather than only as a result of rendering. * Continue chunking by comparing
 the filtered item count to the limit (not the unfiltered item count).

---
 lib/elements/dom-repeat.js       | 111 ++++++---
 test/unit/dom-repeat-elements.js |   2 +-
 test/unit/dom-repeat.html        | 384 +++++++++++++++++--------------
 3 files changed, 289 insertions(+), 208 deletions(-)

diff --git a/lib/elements/dom-repeat.js b/lib/elements/dom-repeat.js
index c819b2c199..33d6d4d102 100644
--- a/lib/elements/dom-repeat.js
+++ b/lib/elements/dom-repeat.js
@@ -247,13 +247,13 @@ export class DomRepeat extends domRepeatBase {
       /**
        * Defines an initial count of template instances to render after setting
        * the `items` array, before the next paint, and puts the `dom-repeat`
-       * into "chunking mode".  The remaining items will be created and rendered
-       * incrementally at each animation frame therof until all instances have
+       * into "chunking mode".  The remaining items (and any future items as a
+       * result of pushing onto the array) will be created and rendered
+       * incrementally at each animation frame thereof until all instances have
        * been rendered.
        */
       initialCount: {
-        type: Number,
-        observer: '__initializeChunking'
+        type: Number
       },
 
       /**
@@ -285,6 +285,25 @@ export class DomRepeat extends domRepeatBase {
        */
       notifyDomChange: {
         type: Boolean
+      },
+
+      /**
+       * When chunking is enabled via `initialCount` and the `items` array is
+       * set to a new array, this flag controls whether the previously rendered
+       * instances are reused or not.
+       *
+       * When `true`, any previously rendered template instances are updated in
+       * place to their new item values synchronously in one shot, and then any
+       * further items (if any) are chunked out.  When `false`, the list is
+       * returned back to its `initialCount` (any instances over the initial
+       * count are discarded) and the remainder of the list is chunked back in.
+       * Set this to `true` to avoid re-creating the list and losing scroll
+       * position, although note that when changing the list to completely
+       * different data the render thread will be blocked until all existing
+       * instances are updated to their new data.
+       */
+      reuseChunkedInstances: {
+        type: Boolean
       }
 
     };
@@ -303,7 +322,10 @@ export class DomRepeat extends domRepeatBase {
     this.__renderDebouncer = null;
     this.__itemsIdxToInstIdx = {};
     this.__chunkCount = null;
-    this.__lastChunkTime = null;
+    this.__renderStartTime = null;
+    this.__restartChunking = true;
+    this.__shouldMeasureChunk = false;
+    this.__shouldContinueChunking = false;
     this.__sortFn = null;
     this.__filterFn = null;
     this.__observePaths = null;
@@ -445,36 +467,58 @@ export class DomRepeat extends domRepeatBase {
     return Math.ceil(1000/rate);
   }
 
-  __initializeChunking() {
-    if (this.initialCount) {
-      this.__limit = this.initialCount;
-      this.__chunkCount = this.initialCount;
-      this.__lastChunkTime = performance.now();
+  __updateLimitAndScheduleNextChunk(filteredItemCount) {
+    let newCount;
+    if (this.__restartChunking) {
+      this.__restartChunking = false;
+      // Limit next render to the initial count
+      this.__limit = Math.min(filteredItemCount, this.initialCount);
+      // Subtract off any existing instances to determine the number of
+      // instances that will be created
+      newCount = Math.max(this.__limit - this.__instances.length, 0);
+      // Initialize the chunk size with how many items we're creating
+      this.__chunkCount = newCount || 1;
+    } else {
+      // The number of new instances that will be created is based on the
+      // existing instances, the new list size, and the maximum chunk size
+      newCount = Math.min(
+        Math.max(filteredItemCount - this.__instances.length, 0), 
+        this.__chunkCount);
+      // Increase the limit based on how many new items we're making
+      this.__limit = Math.min(this.__limit + newCount, filteredItemCount);
     }
-  }
-
-  __tryRenderChunk() {
-    // Debounced so that multiple calls through `_render` between animation
-    // frames only queue one new rAF (e.g. array mutation & chunked render)
-    if (this.items && this.__limit < this.items.length) {
-      this.__debounceRender(this.__requestRenderChunk);
+    // Schedule a rAF task to measure the total time to render the chunk
+    // (including layout/paint plus any other thread contention), throttle the
+    // chunk size accordingly, and schedule another render if there is more to
+    // chunk
+    this.__shouldMeasureChunk = newCount === this.__chunkCount;
+    this.__shouldContinueChunking = this.__limit < filteredItemCount;
+    this.__renderStartTime = performance.now();
+    if (this.__shouldMeasureChunk || this.__shouldContinueChunking) {
+      this.__debounceRender(this.__continueChunkingAfterRaf);
     }
   }
 
-  __requestRenderChunk() {
-    requestAnimationFrame(()=>this.__renderChunk());
+  __continueChunkingAfterRaf() {
+    requestAnimationFrame(() => this.__continueChunking());
   }
 
-  __renderChunk() {
+  __continueChunking() {
     // Simple auto chunkSize throttling algorithm based on feedback loop:
-    // measure actual time between frames and scale chunk count by ratio
-    // of target/actual frame time
-    let currChunkTime = performance.now();
-    let ratio = this._targetFrameTime / (currChunkTime - this.__lastChunkTime);
-    this.__chunkCount = Math.round(this.__chunkCount * ratio) || 1;
-    this.__limit += this.__chunkCount;
-    this.__lastChunkTime = currChunkTime;
-    this.__debounceRender(this.__render);
+    // measure actual time between frames and scale chunk count by ratio of
+    // target/actual frame time.  Only modify chunk size if our measurement
+    // reflects the cost of a creating a full chunk's worth of instances; this
+    // avoids scaling up the chunk size if we e.g. quickly re-rendered instances
+    // in place
+    if (this.__shouldMeasureChunk) {
+      const renderTime = performance.now() - this.__renderStartTime;
+      const ratio = this._targetFrameTime / renderTime;
+      this.__chunkCount = Math.round(this.__chunkCount * ratio) || 1;
+    }
+    // Enqueue a new render if we haven't reached the full size of the list
+    if (this.__shouldContinueChunking) {
+      this.__debounceRender(this.__render);
+    }
   }
 
   __observeChanged() {
@@ -491,7 +535,9 @@ export class DomRepeat extends domRepeatBase {
     if (!this.__handleItemPath(change.path, change.value)) {
       // Otherwise, the array was reset ('items') or spliced ('items.splices'),
       // so queue a full refresh
-      this.__initializeChunking();
+      if (change.path === 'items' && !this.reuseChunkedInstances) {
+        this.__restartChunking = true;
+      }
       this.__debounceRender(this.__render);
     }
   }
@@ -561,8 +607,6 @@ export class DomRepeat extends domRepeatBase {
         composed: true
       }));
     }
-    // Check to see if we need to render more items
-    this.__tryRenderChunk();
   }
 
   __applyFullRefresh() {
@@ -580,6 +624,11 @@ export class DomRepeat extends domRepeatBase {
     if (this.__sortFn) {
       isntIdxToItemsIdx.sort((a, b) => this.__sortFn(items[a], items[b]));
     }
+    // If we're chunking, increase the limit if there are new instances to
+    // create and schedule the next chunk
+    if (this.initialCount) {
+      this.__updateLimitAndScheduleNextChunk(isntIdxToItemsIdx.length);
+    }
     // items->inst map kept for item path forwarding
     const itemsIdxToInstIdx = this.__itemsIdxToInstIdx = {};
     let instIdx = 0;
diff --git a/test/unit/dom-repeat-elements.js b/test/unit/dom-repeat-elements.js
index bc10f972f1..9455a89910 100644
--- a/test/unit/dom-repeat-elements.js
+++ b/test/unit/dom-repeat-elements.js
@@ -434,7 +434,7 @@ Polymer({
 });
 Polymer({
   _template: html`
-    <template id="repeater" is="dom-repeat" items="{{items}}" initial-count="10">
+    <template id="repeater" is="dom-repeat" items="{{items}}" initial-count="10" target-framerate="25">
       <x-wait>{{item.prop}}</x-wait>
     </template>
 `,
diff --git a/test/unit/dom-repeat.html b/test/unit/dom-repeat.html
index 5788df62c5..9dce86e49d 100644
--- a/test/unit/dom-repeat.html
+++ b/test/unit/dom-repeat.html
@@ -121,9 +121,6 @@ <h4>inDocumentRepeater</h4>
   <h4>x-repeat-limit</h4>
   <x-repeat-limit id="limited"></x-repeat-limit>
 
-  <h4>x-repeat-chunked</h4>
-  <x-repeat-chunked id="chunked"></x-repeat-chunked>
-
   <div id="inDocumentContainer">
   </div>
 
@@ -141,7 +138,7 @@ <h4>x-repeat-chunked</h4>
 import './dom-repeat-elements.js';
 import { flush } from '../../lib/utils/flush.js';
 import { calculateSplices } from '../../lib/utils/array-splice.js';
-/* global limited inDocumentRepeater inDocumentContainer inDocumentContainerOrig chunked nonUpgrade*/
+/* global limited inDocumentRepeater inDocumentContainer inDocumentContainerOrig nonUpgrade*/
 
 /*
   Expected:
@@ -3948,206 +3945,241 @@ <h4>x-repeat-chunked</h4>
 
 });
 
-// TODO(kschaaf): This test suite has proven to be flaky only on IE, only
-// on CI (Sauce) presumably because of rAF handling in the CI environment
-// disabling for IE for now to avoid Polymer tests being flaky
-if (!/Trident/.test(navigator.userAgent)) {
+suite('chunked rendering', function() {
 
-  // TODO(kschaaf): We're experiencing more flakiness on Safari on CI,
-  // for now, skip these tests and revisit
-  suite.skip('chunked rendering', function() {
+  let chunked;
+  let verifyAfterChange;
+  const verify = () => verifyAfterChange && verifyAfterChange();
+  setup(() => {
+    chunked = document.createElement('x-repeat-chunked');
+    chunked.addEventListener('dom-change', verify);
+    document.body.appendChild(chunked);
+  });
+  teardown(() => {
+    chunked.removeEventListener('dom-change', verify);
+    document.body.removeChild(chunked);
+    chunked = null;
+    verifyAfterChange = null;
+  });
 
-    test('basic chunked rendering', function(done) {
+  // Framerate=25, element cost = 4ms: should never make more than
+  // (1000/25) / 4 = 10 elements per frame
+  const MAX_PER_FRAME = (1000 / 25) / 4;
 
-      var checkItemOrder = function(stamped) {
-        for (var i=0; i<stamped.length; i++) {
-          assert.equal(stamped[i].textContent, i);
-        }
-      };
+  test('basic chunked rendering', function(done) {
 
-      var lastLength = 0;
-      var checkCount = function() {
-        var stamped = chunked.root.querySelectorAll('*:not(template):not(dom-repeat)');
-        checkItemOrder(stamped);
-        if (stamped.length && lastLength === 0) {
-          // Initial rendering of initial count
-          assert.equal(stamped.length, 10);
-        } else {
-          // Remaining rendering increments
-          assert.isTrue(stamped.length > lastLength);
-        }
-        if (stamped.length < 100) {
-          lastLength = stamped.length;
-          checkUntilComplete();
-        } else {
-          // Final rendering at exact item count
-          assert.equal(stamped.length, 100);
-          done();
-        }
-      };
-      var checkUntilComplete = function() {
-        // On polyfilled MO, need to wait one setTimeout before rAF
-        if (MutationObserver._isPolyfilled) {
-          setTimeout(function() {
-            requestAnimationFrame(checkCount);
-          });
-        } else {
-          requestAnimationFrame(checkCount);
-        }
-      };
+    var checkItemOrder = function(stamped) {
+      for (var i=0; i<stamped.length; i++) {
+        assert.equal(stamped[i].textContent, i);
+      }
+    };
 
-      chunked.items = chunked.preppedItems.slice();
-      checkUntilComplete();
+    let lastStamped;
+    let frameCount = 0;
+    verifyAfterChange = function() {
+      var stamped = Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)'));
+      checkItemOrder(stamped);
+      if (!lastStamped) {
+        // Initial rendering of initial count
+        assert.equal(stamped.length, 10);
+      } else {
+        // Remaining rendering increments
+        assert.isTrue(stamped.length > lastStamped.length,
+          'list instance count should increase each frame');
+        assert.deepEqual(lastStamped, stamped.slice(0, lastStamped.length),
+          'list should not re-render instances during mutation');
+        assert.isAtMost((stamped.length - lastStamped.length), MAX_PER_FRAME,
+          `list should not render more than ${MAX_PER_FRAME} per frame`);
+      }
+      if (stamped.length < chunked.items.length) {
+        frameCount++;
+        lastStamped = stamped;
+      } else {
+        // Final rendering at exact item count
+        assert.equal(stamped.length, 100, 'final count wrong');
+        assert.isAtLeast(frameCount, 10, 'should have taken at least 10 frames to render');
+        done();
+      }
+    };
+    chunked.items = chunked.preppedItems.slice();
 
-    });
+  });
 
-    test('mutations during chunked rendering', function(done) {
+  test('mutations during chunked rendering', function(done) {
 
-      var checkItemOrder = function(stamped) {
-        var last = -1;
-        for (var i=0; i<stamped.length; i++) {
-          var curr = parseFloat(stamped[i].textContent);
-          assert.isTrue(curr > last);
-          last = curr;
-        }
-      };
+    var checkItemOrder = function(stamped) {
+      var last = -1;
+      for (var i=0; i<stamped.length; i++) {
+        var curr = parseFloat(stamped[i].textContent);
+        assert.isTrue(curr > last);
+        last = curr;
+      }
+    };
 
-      var mutateArray = function(repeater, renderedCount) {
-        // The goal here is to remove & add some, and do it over
-        // the threshold of where we have currently rendered items, and
-        // ensure that the prop values of the newly inserted items are in
-        // ascending order so we can do a simple check in checkItemOrder
-        var overlap = 2;
-        var remove = 4;
-        var add = 6;
-        var start = renderedCount.length - overlap;
-        if (start + add < repeater.items.length) {
-          var end = start + remove;
-          var args = ['items', start, remove];
-          var startVal = repeater.items[start].prop;
-          var endVal = repeater.items[end].prop;
-          var delta = (endVal - startVal) / add;
-          for (var i=0; i<add; i++) {
-            args.push({prop: startVal + i*delta});
-          }
-          repeater.splice.apply(repeater, args);
+    var mutateArray = function(repeater, renderedCount) {
+      // The goal here is to remove & add some, and do it over
+      // the threshold of where we have currently rendered items, and
+      // ensure that the prop values of the newly inserted items are in
+      // ascending order so we can do a simple check in checkItemOrder
+      var overlap = 2;
+      var remove = 4;
+      var add = 6;
+      var start = renderedCount.length - overlap;
+      if (start + add < repeater.items.length) {
+        var end = start + remove;
+        var args = ['items', start, remove];
+        var startVal = repeater.items[start].prop;
+        var endVal = repeater.items[end].prop;
+        var delta = (endVal - startVal) / add;
+        for (var i=0; i<add; i++) {
+          args.push({prop: startVal + i*delta});
         }
-      };
+        repeater.splice.apply(repeater, args);
+      }
+    };
 
-      var lastLength = 0;
-      var mutateCount = 5;
-      var checkCount = function() {
-        var stamped = chunked.root.querySelectorAll('*:not(template):not(dom-repeat)');
-        checkItemOrder(stamped);
-        if (stamped.length && lastLength === 0) {
-          // Initial rendering of initial count
-          assert.equal(stamped.length, 10);
-        } else {
-          // Remaining rendering increments
-          assert.isTrue(stamped.length > lastLength);
-        }
-        if (stamped.length < chunked.items.length) {
-          if (mutateCount-- > 0) {
-            mutateArray(chunked, stamped);
-          }
-          lastLength = stamped.length;
-          checkUntilComplete();
-        } else {
-          // Final rendering at exact item count
-          assert.equal(stamped.length, chunked.items.length);
-          done();
-        }
-      };
-      var checkUntilComplete = function() {
-        // On polyfilled MO, need to wait one setTimeout before rAF
-        if (MutationObserver._isPolyfilled) {
-          setTimeout(function() {
-            requestAnimationFrame(checkCount);
-          });
-        } else {
-          requestAnimationFrame(checkCount);
+    let lastStamped;
+    var frameCount = 0;
+    verifyAfterChange = function() {
+      var stamped = Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)'));
+      checkItemOrder(stamped);
+      if (!lastStamped) {
+        // Initial rendering of initial count
+        assert.equal(stamped.length, 10);
+      } else {
+        // Remaining rendering increments
+        assert.isTrue(stamped.length > lastStamped.length,
+          'list instance count should increase each frame');
+        assert.deepEqual(lastStamped, stamped.slice(0, lastStamped.length),
+          'list should not re-render instances during mutation');
+        assert.isAtMost((stamped.length - lastStamped.length), MAX_PER_FRAME,
+          `list should not render more than ${MAX_PER_FRAME} per frame`);
+      }
+      if (stamped.length < chunked.items.length) {
+        if (frameCount++ < 5) {
+          mutateArray(chunked, stamped);
         }
-      };
+        lastStamped = stamped;
+      } else {
+        // Final rendering at exact item count
+        assert.equal(stamped.length, chunked.items.length, 'final count wrong');
+        assert.isAtLeast(frameCount, 10, 'should have taken at least 10 frames to render');
+        done();
+      }
+    };
+    chunked.items = chunked.preppedItems.slice();
 
-      chunked.items = chunked.preppedItems.slice();
-      checkUntilComplete();
+  });
 
-    });
 
+  test('mutations during chunked rendering, sort & filtered', function(done) {
 
-    test('mutations during chunked rendering, sort & filtered', function(done) {
+    var checkItemOrder = function(stamped) {
+      var last = Infinity;
+      for (var i=0; i<stamped.length; i++) {
+        var curr = parseFloat(stamped[i].textContent);
+        assert.isTrue(curr <= last);
+        assert.strictEqual(curr % 2, 0);
+        last = curr;
+      }
+    };
 
-      var checkItemOrder = function(stamped) {
-        var last = Infinity;
-        for (var i=0; i<stamped.length; i++) {
-          var curr = parseFloat(stamped[i].textContent);
-          assert.isTrue(curr <= last);
-          assert.strictEqual(curr % 2, 0);
-          last = curr;
-        }
-      };
+    var mutateArray = function(repeater, stamped) {
+      var start = parseInt(stamped[0].textContent);
+      var end = parseInt(stamped[stamped.length-1].textContent);
+      var mid = (end-start)/2;
+      for (var i=0; i<5; i++) {
+        chunked.push('items', {prop: mid + 1});
+      }
+      chunked.splice('items', Math.round(stamped.length/2), 3);
+    };
 
-      var mutateArray = function(repeater, stamped) {
-        var start = parseInt(stamped[0].textContent);
-        var end = parseInt(stamped[stamped.length-1].textContent);
-        var mid = (end-start)/2;
-        for (var i=0; i<5; i++) {
-          chunked.push('items', {prop: mid + 1});
+    var lastStamped;
+    var frameCount = 0;
+    verifyAfterChange = function() {
+      var stamped = Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)'));
+      checkItemOrder(stamped);
+      var filteredLength = chunked.items.filter(chunked.$.repeater.filter).length;
+      if (!lastStamped) {
+        // Initial rendering of initial count
+        assert.equal(stamped.length, 10);
+      } else {
+        // Remaining rendering increments
+        assert.isTrue(stamped.length > lastStamped.length,
+          'list instance count should increase each frame');
+        assert.deepEqual(lastStamped, stamped.slice(0, lastStamped.length),
+          'list should not re-render instances during mutation');
+        assert.isAtMost((stamped.length - lastStamped.length), MAX_PER_FRAME,
+          `list should not render more than ${MAX_PER_FRAME} per frame`);
+      }
+      if (stamped.length < filteredLength) {
+        if (frameCount++ < 4) {
+          mutateArray(chunked, stamped);
         }
-        chunked.splice('items', Math.round(stamped.length/2), 3);
-      };
+        lastStamped = stamped;
+      } else {
+        assert.equal(stamped.length, filteredLength, 'final count wrong');
+        assert.isAtLeast(frameCount, 5, 'should have taken at least 5 frames to render');
+        done();
+      }
+    };
+    chunked.$.repeater.sort = function(a, b) {
+      return b.prop - a.prop;
+    };
+    chunked.$.repeater.filter = function(a) {
+      return (a.prop % 2) === 0;
+    };
+    chunked.items = chunked.preppedItems.slice();
 
-      var lastLength = 0;
-      var mutateCount = 5;
-      var checkCount = function() {
-        var stamped = chunked.root.querySelectorAll('*:not(template):not(dom-repeat)');
-        checkItemOrder(stamped);
-        var filteredLength = chunked.items.filter(chunked.$.repeater.filter).length;
-        if (stamped.length && lastLength === 0) {
-          // Initial rendering of initial count
-          assert.equal(stamped.length, 10);
-        } else {
-          // Remaining rendering increments
-          if (stamped.length < filteredLength) {
-            assert.isTrue(stamped.length > lastLength);
+  });
+
+  suite('resetting items array', () => {
+
+    [false, true].forEach(reuse => {
+
+      test(`reuseChunkedInstances=${reuse}`, (done) => {
+
+        let i = 3;
+        let lastStamped;
+        verifyAfterChange = function() {
+          var stamped = Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)'));
+          if (!lastStamped) {
+            // Initial rendering of initial count
+            assert.equal(stamped.length, 10);
+          } else {
+            // Remaining rendering increments
+            assert.isTrue(stamped.length > lastStamped.length,
+              'list instance count should increase each frame');
+            assert.deepEqual(lastStamped, stamped.slice(0, lastStamped.length),
+              'list should not re-render instances during mutation');
+            assert.isAtMost((stamped.length - lastStamped.length), MAX_PER_FRAME,
+              `list should not render more than ${MAX_PER_FRAME} per frame`);
           }
-        }
-        if (stamped.length < filteredLength) {
-          if (mutateCount-- > 0) {
-            mutateArray(chunked, stamped);
+          if (stamped.length < chunked.items.length) {
+            lastStamped = stamped;
+          } else {
+            assert.equal(stamped.length, chunked.items.length, 'final count wrong');
+            if (--i > 0) {
+              if (!reuse) {
+                lastStamped = null;
+              }
+              chunked.items = chunked.preppedItems.slice();
+            } else {
+              done();
+            }
           }
-          lastLength = stamped.length;
-          checkUntilComplete();
-        } else {
-          assert.equal(stamped.length, filteredLength);
-          done();
-        }
-      };
-      var checkUntilComplete = function() {
-        // On polyfilled MO, need to wait one setTimeout before rAF
-        if (MutationObserver._isPolyfilled) {
-          setTimeout(function() {
-            requestAnimationFrame(checkCount);
-          });
-        } else {
-          requestAnimationFrame(checkCount);
-        }
-      };
+        };
 
-      chunked.$.repeater.sort = function(a, b) {
-        return b.prop - a.prop;
-      };
-      chunked.$.repeater.filter = function(a) {
-        return (a.prop % 2) === 0;
-      };
-      chunked.items = chunked.preppedItems.slice();
-      checkUntilComplete();
+        chunked.$.repeater.reuseChunkedInstances = reuse;
+        chunked.items = chunked.preppedItems.slice();
+
+      });
 
     });
 
   });
 
-}
+});
 
 suite('misc', function() {