Skip to content
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

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

Merged
merged 2 commits into from
May 4, 2017
Merged

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

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
88e5642
perf(virtual-list): relaxed restrictions for fast path
manucorporat Apr 20, 2017
8baa647
perf(virtual-scroll): fast path for removing
manucorporat Apr 23, 2017
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
31 changes: 16 additions & 15 deletions src/components/virtual-scroll/test/basic/app.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,7 @@ export class E2EPage {
webview: string = '';
counter: number = 0;

constructor(plt: Platform, public navCtrl: NavController) {
if (plt.is('ios')) {
if (plt.testUserAgent('Safari')) {
this.webview = ': iOS Safari';

} else if (!!(window as any)['webkit']) {
this.webview = ': iOS WKWebView';

} else {
this.webview = ': iOS UIWebView';
}
}
}
constructor(plt: Platform, public navCtrl: NavController) {}

addItems() {
if (this.items.length === 0) {
Expand Down Expand Up @@ -54,8 +42,21 @@ export class E2EPage {
this.counter++;
}

reload() {
window.location.reload(true);
addRandomItem() {
const index = Math.floor(Math.random() * this.items.length);
console.log('Adding to index: ', index);
this.items.splice( index, 0, {
value: Math.floor(Math.random() * 10000),
someMethod: function() {
return '!!';
}
});
}

changeItem() {
const index = Math.floor(Math.random() * this.items.length);
console.log('Change to index: ', index);
this.items[index] = { value: Math.floor(Math.random() * 10000), someMethod: () => '!!' };
}

trackByFn(index: number, item: any) {
Expand Down
10 changes: 6 additions & 4 deletions src/components/virtual-scroll/test/basic/main.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
<ion-header>
<ion-navbar>
<ion-title>Virtual Scroll{{webview}}</ion-title>
<ion-buttons end>
<button ion-button (click)="reload()">
Reload
<button ion-button icon-only (click)="changeItem()">
Change random
</button>
<button ion-button icon-only (click)="addRandomItem()">
Add random
</button>
<button ion-button icon-only (click)="addItem()">
<ion-icon name="add"></ion-icon>
Append
</button>
</ion-buttons>
</ion-navbar>
Expand Down
65 changes: 53 additions & 12 deletions src/components/virtual-scroll/virtual-scroll.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,8 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
scrollTop: 0,
};
_queue: number = SCROLL_QUEUE_NO_CHANGES;
_recordSize: number = 0;


_virtualTrackBy: TrackByFn;

@ContentChild(VirtualItem) _itmTmp: VirtualItem;
Expand Down Expand Up @@ -379,7 +380,6 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
return this._virtualTrackBy;
}


constructor(
private _iterableDiffers: IterableDiffers,
private _elementRef: ElementRef,
Expand Down Expand Up @@ -412,6 +412,22 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
});
}

/**
* @hidden
*/
firstRecord(): number {
const cells = this._cells;
return (cells.length > 0) ? cells[0].record : 0;
}

/**
* @hidden
*/
lastRecord(): number {
const cells = this._cells;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return (cells.length > 0) ? cells[cells.length - 1].record : 0;
}

/**
* @hidden
*/
Expand All @@ -429,24 +445,38 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {

let needClean = false;
if (changes) {
changes.forEachOperation((item, _, cindex) => {
if (item.previousIndex != null || (cindex < this._recordSize)) {
var lastRecord = this.lastRecord() + 1;

changes.forEachOperation((_, pindex, cindex) => {

// add new record after current position
if (pindex === null && (cindex < lastRecord)) {
console.debug('adding record before current position, slow path');
needClean = true;
return;
}
// remove record after current position
if (pindex < lastRecord && cindex === null) {
console.debug('removing record before current position, slow path');
needClean = true;
return;
}
});
} else {
needClean = true;
}
this._recordSize = this._records.length;

this.readUpdate(needClean);
this.writeUpdate(needClean);
}

/**
* @hidden
*/
readUpdate(needClean: boolean) {
if (needClean) {
// reset everything
console.debug(`virtual-scroll, readUpdate: slow path`);
console.debug('virtual-scroll, readUpdate: slow path');
this._cells.length = 0;
this._nodes.length = 0;
this._itmTmp.viewContainer.clear();
Expand All @@ -458,8 +488,11 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}
}

/**
* @hidden
*/
writeUpdate(needClean: boolean) {
console.debug(`virtual-scroll, writeUpdate`);
console.debug('virtual-scroll, writeUpdate need clean:', needClean);
const data = this._data;
const stopAtHeight = (data.scrollTop + data.renderHeight);
data.scrollDiff = SCROLL_DIFFERENCE_MINIMUM + 1;
Expand All @@ -475,6 +508,9 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
this.renderVirtual(needClean);
}

/**
* @hidden
*/
private calcDimensions() {
calcDimensions(this._data, this._elementRef.nativeElement,
this.approxItemWidth, this.approxItemHeight,
Expand Down Expand Up @@ -524,7 +560,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
this._itmTmp.viewContainer,
this._itmTmp.templateRef,
this._hdrTmp && this._hdrTmp.templateRef,
this._ftrTmp && this._ftrTmp.templateRef, needClean,
this._ftrTmp && this._ftrTmp.templateRef, needClean
);
});

Expand Down Expand Up @@ -571,7 +607,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}

/**
* @private
* @hidden
*/
resize() {
// only continue if we've already initialized
Expand Down Expand Up @@ -605,7 +641,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}

/**
* @private
* @hidden
*/
private _stepChangeDetection() {
// we need to do some change detection in this frame
Expand All @@ -624,7 +660,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}

/**
* @private
* @hidden
*/
private _stepNoChanges() {
const data = this._data;
Expand Down Expand Up @@ -675,7 +711,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}

/**
* @private
* @hidden
*/
scrollUpdate(ev: ScrollEvent) {
// set the scroll top from the scroll event
Expand Down Expand Up @@ -718,6 +754,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}

/**
* @hidden
* NO DOM
*/
private _listeners() {
Expand All @@ -738,6 +775,7 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}

/**
* @hidden
* DOM WRITE
*/
private _setHeight(newVirtualHeight: number) {
Expand All @@ -762,6 +800,9 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}
}

/**
* @hidden
*/
setElementClass(className: string, add: boolean) {
this._renderer.setElementClass(this._elementRef.nativeElement, className, add);
}
Expand Down