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.

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

icons rendering performance too slow. #7825

Open
maxtorchel opened this issue Feb 6, 2023 · 3 comments · May be fixed by #8579
Open

icons rendering performance too slow. #7825

maxtorchel opened this issue Feb 6, 2023 · 3 comments · May be fixed by #8579

Comments

@maxtorchel
Copy link

maxtorchel commented Feb 6, 2023

Reproduction link

https://stackblitz.com/edit/ng-zorro-antd-ivy-ddpwnf?file=src/app/app.component.ts

Steps to reproduce

Press Reload button on stackblitz example.

What is expected?

Reload button freezes page abour 2-3 seconds.

What is actually happening?

Reload button freezes page about 25 seconds.

Environment Info
ng-zorro-antd 15.0.2
Browser firefox 109

When i have table with large page size, and i have a column with icons, page rendering goes to slow. On stackblitz all works fine on default 13 version, but locally under latest versions it slow down.

@maxtorchel
Copy link
Author

maxtorchel commented Feb 8, 2023

after update stackblitz dependencies to latest, this bug appear on it, so under version 13 it works normal, but under latest 15 it slow down.
i found this in change log:
icon: re-enter Angular zone after icons have been loaded [#7719] [754ded6]
maybe it happens because of this fix?

@arturovt
Copy link
Member

arturovt commented Mar 3, 2023

The re-enter Angular zone commit was a forward fix for the previous issue where icon re-rendering (where detectChanges() is called) affected other parts of the template because it was called outside of the Angular zone. I was trying to reduce the change detection cycles triggered by a "load + render icon" flow. I could only reduce this number by wrapping changeIcon into runOutsideAngular so any asynchronous stuff happening while the promise is being resolved doesn't force Angular to run tick(). The issue you're talking about has existed before any zone-related changes have been made to the nz-icon directive.

I tested the code locally and it takes 23 seconds for me to render after clicking "Reload". I was able to write the solution where it takes only ~7 s to render:

Screenshot from 2023-03-03 18-42-34

That's not the best result. But this is x3 boost compared to 23 seconds.

I had to edit the code directly:

// ng-zorro-antd/fesm2020/ng-zorro-antd-icon.mjs

let scheduler = null;
class Scheduler {
  constructor(ngZone) {
    this.ngZone = ngZone;
    this.callbacks = new Set();
    this.scheduled = false;
  }

  schedule(callback) {
    this.callbacks.add(callback);

    if (this.scheduled) {
      return;
    }

    this.scheduled = true;

    requestAnimationFrame(() => {
      if (this.callbacks.size !== 0) {
        this.ngZone.run(() => {
          for (const callback of this.callbacks.values()) {
            callback();
          }
          this.callbacks.clear();
        });
      }

      this.scheduled = false;
    });
  }

  static create(ngZone) {
    return scheduler || (scheduler = new Scheduler(ngZone));
  }
}

class NzIconDirective extends IconDirective {
  constructor(
    ngZone,
    changeDetectorRef,
    elementRef,
    iconService,
    renderer,
    iconPatch
  ) {
    super(iconService, elementRef, renderer);
    this.ngZone = ngZone;
    // ...
    this.scheduler = Scheduler.create(ngZone);
    // ...
  }

  changeIcon2() {
    this.setClassName();
    this.ngZone.runOutsideAngular(() => {
      from(this._changeIcon())
        .pipe(takeUntil(this.destroy$))
        .subscribe({
          next: (svgOrRemove) => {
            this.scheduler.schedule(() => {
              this.changeDetectorRef.detectChanges();
              if (svgOrRemove) {
                this.setSVGData(svgOrRemove);
                this.handleSpin(svgOrRemove);
                this.handleRotate(svgOrRemove);
              }
            });
          },
          error: warn,
        });
    });
  }
}

This is the most basic scheduler implementation. In practice we should be more flexible and don't allow the scheduler to keep more than say 40-50 tasks waiting to be executed. So if callbacks.size becomes greater than 40, we should flush callbacks and start scheduling again.

@pafflu
Copy link

pafflu commented Apr 2, 2023

In my app it's come to the point where I have to replace icons with words or emojis even, because the performance for icons in lists or tables is just too slow.
Is this being worked on? Is there another issue that tracks the overall performance problems with the IconDirective?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants