Skip to content

Fixes for several related dom-repeat chunking issues. Fixes #5631. #5632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 22, 2020
Merged

Fixes for several related dom-repeat chunking issues. Fixes #5631. #5632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b40840b
Fixes for several related dom-repeat chunking issues. Fixes #5631.
kevinpschaaf Feb 21, 2020
0797488
Updates from review.
kevinpschaaf Feb 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 117 additions & 78 deletions lib/elements/dom-repeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
},

/**
Expand Down Expand Up @@ -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
}

};
Expand All @@ -299,11 +318,13 @@ export class DomRepeat extends domRepeatBase {
super();
this.__instances = [];
this.__limit = Infinity;
this.__pool = [];
this.__renderDebouncer = null;
this.__itemsIdxToInstIdx = {};
this.__chunkCount = null;
this.__lastChunkTime = null;
this.__renderStartTime = null;
this.__itemsArrayChanged = false;
this.__shouldMeasureChunk = false;
this.__shouldContinueChunking = false;
this.__sortFn = null;
this.__filterFn = null;
this.__observePaths = null;
Expand Down Expand Up @@ -445,57 +466,11 @@ 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();
}
}

__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);
}
}

__requestRenderChunk() {
requestAnimationFrame(()=>this.__renderChunk());
}

__renderChunk() {
// 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);
}

__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
this.__initializeChunking();
this.__debounceRender(this.__render);
}
}

__handleObservedPaths(path) {
// Handle cases where path changes should cause a re-sort/filter
if (this.__sortFn || this.__filterFn) {
Expand All @@ -514,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.
Expand Down Expand Up @@ -545,28 +537,34 @@ 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', {
bubbles: true,
composed: true
}));
}
// Check to see if we need to render more items
this.__tryRenderChunk();
}

__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<items.length; i++) {
isntIdxToItemsIdx[i] = i;
Expand All @@ -580,6 +578,60 @@ export class DomRepeat extends domRepeatBase {
if (this.__sortFn) {
isntIdxToItemsIdx.sort((a, b) => this.__sortFn(items[a], items[b]));
}
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;
Expand Down Expand Up @@ -622,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);
}

Expand All @@ -638,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);
Expand Down
2 changes: 1 addition & 1 deletion test/unit/dom-repeat-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
`,
Expand Down
Loading