-
Notifications
You must be signed in to change notification settings - Fork 159
Refactor virtualization resize observer to track individual forOf elements. #16809
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.
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
base: master
Are you sure you want to change the base?
Conversation
projects/igniteui-angular-performance/src/app/hierarchical-grid/hierarchical-grid.component.ts
Fixed
Show fixed
Hide fixed
projects/igniteui-angular-performance/src/app/hierarchical-grid/hierarchical-grid.component.ts
Fixed
Show fixed
Hide fixed
…/IgniteUI/igniteui-angular into mkirova/resize-observer-refactor
rkaraivanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think we can squeeze out a little bit more perf with several of the points in this review.
| protected platformUtil = inject(PlatformUtil); | ||
| protected document = inject(DOCUMENT); | ||
| private _igxForOf: U & T[] | null = null; | ||
| protected _embeddedViewSizesCache = new Map<EmbeddedViewRef<any>, number>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| protected _embeddedViewSizesCache = new Map<EmbeddedViewRef<any>, number>(); | |
| protected _embeddedViewSizesCache = new WeakMap<EmbeddedViewRef<any>, number>(); |
I don't see the viewSizesCache being iterated, so let's go with the more memory-friendly version.
|
|
||
| for (let index = 0; index < this._embeddedViews.length; index++) { | ||
| const targetIndex = this.state.startIndex + index; | ||
| const nodeSize = this.getEmbeddedViewSize(index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getEmbeddedViewSize calls get embeddedViewNodes which creates a new array and iterates all views on each access to the getter. This is O(n²) operations at least. I suggest to cache the embeddedViewNodes outside the loop and adjust the calculations based on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you drop the O(n) reduce operation?
Can you update the _virtSize with the totalDiff directly?
Something like:
this._virtSize += totalDiff;
if (this._virtSize > this._maxSize) {
this._virtRatio = this._virtSize / this._maxSize;
}
this.scrollComponent.size = Math.min(this.scrollComponent.size + totalDiff, this._maxSize);
if (!this.scrollComponent.destroyed) {
this.scrollComponent.cdr.detectChanges();
}|
|
||
| protected updateViewSizes(entries:ResizeObserverEntry[] ) { | ||
| entries.forEach((entry) => { | ||
| const index = parseInt(entry.target.getAttribute('data-index'), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const index = parseInt(entry.target.getAttribute('data-index'), 0); | |
| const index = parseInt(entry.target.getAttribute('data-index'), 10); |
A typo
| } | ||
|
|
||
| protected updateViewSizes(entries:ResizeObserverEntry[] ) { | ||
| entries.forEach((entry) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a simple for of loop instead of a forEach?
Closes #16821
Attach observer to individual root node views so that they track any change individually for that view. Update size accordingly.
Cache sizes of embedded views and use the cache when updating virtualization caches on scroll.
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)