From 0797488bcc3ba081f90a8abf3cf3cb98046023d0 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Fri, 21 Feb 2020 16:07:02 -0800 Subject: [PATCH] Updates from review. * Refactoring `__render` for readability * Removing `__pool`; this was never used in v2: since we reset the pool every update and items are only ever pushed at detach time and we only detach at the end of updates (as opposed to v1 which had more sophisticated splicing) --- lib/elements/dom-repeat.js | 190 ++++++++++++++++++------------------- 1 file changed, 90 insertions(+), 100 deletions(-) diff --git a/lib/elements/dom-repeat.js b/lib/elements/dom-repeat.js index 33d6d4d102..0a88c81d0f 100644 --- a/lib/elements/dom-repeat.js +++ b/lib/elements/dom-repeat.js @@ -318,12 +318,11 @@ export class DomRepeat extends domRepeatBase { super(); this.__instances = []; this.__limit = Infinity; - this.__pool = []; this.__renderDebouncer = null; this.__itemsIdxToInstIdx = {}; this.__chunkCount = null; this.__renderStartTime = null; - this.__restartChunking = true; + this.__itemsArrayChanged = false; this.__shouldMeasureChunk = false; this.__shouldContinueChunking = false; this.__sortFn = null; @@ -467,81 +466,11 @@ export class DomRepeat extends domRepeatBase { return Math.ceil(1000/rate); } - __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); - } - // 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); - } - } - - __continueChunkingAfterRaf() { - requestAnimationFrame(() => this.__continueChunking()); - } - - __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. 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() { this.__observePaths = this.observe && this.observe.replace('.*', '.').split(' '); } - __itemsChanged(change) { - if (this.items && !Array.isArray(this.items)) { - console.warn('dom-repeat expected array for `items`, found', this.items); - } - // If path was to an item (e.g. 'items.3' or 'items.3.foo'), forward the - // path to that instance synchronously (returns false for non-item paths) - if (!this.__handleItemPath(change.path, change.value)) { - // Otherwise, the array was reset ('items') or spliced ('items.splices'), - // so queue a full refresh - if (change.path === 'items' && !this.reuseChunkedInstances) { - this.__restartChunking = true; - } - this.__debounceRender(this.__render); - } - } - __handleObservedPaths(path) { // Handle cases where path changes should cause a re-sort/filter if (this.__sortFn || this.__filterFn) { @@ -560,6 +489,23 @@ export class DomRepeat extends domRepeatBase { } } + __itemsChanged(change) { + if (this.items && !Array.isArray(this.items)) { + console.warn('dom-repeat expected array for `items`, found', this.items); + } + // If path was to an item (e.g. 'items.3' or 'items.3.foo'), forward the + // path to that instance synchronously (returns false for non-item paths) + if (!this.__handleItemPath(change.path, change.value)) { + // Otherwise, the array was reset ('items') or spliced ('items.splices'), + // so queue a render. Restart chunking when the items changed (for + // backward compatibility), unless `reuseChunkedInstances` option is set + if (change.path === 'items') { + this.__itemsArrayChanged = true; + } + this.__debounceRender(this.__render); + } + } + /** * @param {function(this:DomRepeat)} fn Function to debounce. * @param {number=} delay Delay in ms to debounce by. @@ -591,15 +537,23 @@ export class DomRepeat extends domRepeatBase { // No template found yet return; } - this.__applyFullRefresh(); - // Reset the pool - // TODO(kschaaf): Reuse pool across turns and nested templates - // Now that objects/arrays are re-evaluated when set, we can safely - // reuse pooled instances across turns, however we still need to decide - // semantics regarding how long to hold, how many to hold, etc. - this.__pool.length = 0; + let items = this.items || []; + // Sort and filter the items into a mapping array from instance->item + const isntIdxToItemsIdx = this.__sortAndFilterItems(items); + // If we're chunking, increase the limit if there are new instances to + // create and schedule the next chunk + if (this.initialCount) { + this.__updateLimit(isntIdxToItemsIdx.length); + } + // Create, update, and/or remove instances + this.__updateInstances(items, isntIdxToItemsIdx); // Set rendered item count this._setRenderedItemCount(this.__instances.length); + // If we're chunking, schedule a rAF task to measure/continue chunking + if (this.initialCount && + (this.__shouldMeasureChunk || this.__shouldContinueChunking)) { + this.__debounceRender(this.__continueChunkingAfterRaf); + } // Notify users if (!suppressTemplateNotifications || this.notifyDomChange) { this.dispatchEvent(new CustomEvent('dom-change', { @@ -609,8 +563,8 @@ export class DomRepeat extends domRepeatBase { } } - __applyFullRefresh() { - let items = this.items || []; + __sortAndFilterItems(items) { + // Generate array maping the instance index to the items array index let isntIdxToItemsIdx = new Array(items.length); for (let i=0; i 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); + return isntIdxToItemsIdx; + } + + __updateLimit(filteredItemCount) { + let newCount; + if (!this.__chunkCount || + (this.__itemsArrayChanged && !this.reuseChunkedInstances)) { + // 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); + // Update the limit based on how many new items we're making, limited + // buy the total size of the list + this.__limit = Math.min(this.__limit + newCount, filteredItemCount); + } + this.__itemsArrayChanged = false; + // Record some state about chunking for use in `__continueChunking` + this.__shouldMeasureChunk = newCount === this.__chunkCount; + this.__shouldContinueChunking = this.__limit < filteredItemCount; + this.__renderStartTime = performance.now(); + } + + __continueChunkingAfterRaf() { + requestAnimationFrame(() => this.__continueChunking()); + } + + __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. 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); + } + } + + __updateInstances(items, isntIdxToItemsIdx) { // items->inst map kept for item path forwarding const itemsIdxToInstIdx = this.__itemsIdxToInstIdx = {}; let instIdx = 0; @@ -671,10 +674,7 @@ export class DomRepeat extends domRepeatBase { } __detachAndRemoveInstance(idx) { - let inst = this.__detachInstance(idx); - if (inst) { - this.__pool.push(inst); - } + this.__detachInstance(idx); this.__instances.splice(idx, 1); } @@ -687,17 +687,7 @@ export class DomRepeat extends domRepeatBase { } __insertInstance(item, instIdx, itemIdx) { - let inst = this.__pool.pop(); - if (inst) { - // TODO(kschaaf): If the pool is shared across turns, hostProps - // need to be re-set to reused instances in addition to item - inst._setPendingProperty(this.as, item); - inst._setPendingProperty(this.indexAs, instIdx); - inst._setPendingProperty(this.itemsIndexAs, itemIdx); - inst._flushProperties(); - } else { - inst = this.__stampInstance(item, instIdx, itemIdx); - } + const inst = this.__stampInstance(item, instIdx, itemIdx); let beforeRow = this.__instances[instIdx + 1]; let beforeNode = beforeRow ? beforeRow.children[0] : this; wrap(wrap(this).parentNode).insertBefore(inst.root, beforeNode);