swimlane / ngx-charts

:bar_chart: Declarative Charting Framework for Angular
https://swimlane.github.io/ngx-charts/
MIT License
4.28k stars 1.15k forks source link

perf: run logic outside ngZone #942

Open Ploppy3 opened 5 years ago

Ploppy3 commented 5 years ago

I'm submitting a ... (check one with "x")

Current behavior The mouseOver event runs inside the ngZone, which leads to hundreds of Angular check which impacts performance.

Expected behavior Some events must run outside the ngZone, only template changes should run inside the ngZone.

Reproduction of the problem https://stackblitz.com/edit/angular-ks78z1?file=src%2Fapp%2Fapp.component.html

What is the motivation / use case for changing the behavior? Perf matters.

marjan-georgiev commented 5 years ago

The mouseOver event is needed for the tooltip. Any idea how to make that work without a mouseOver event?

Ploppy3 commented 5 years ago

The event is not the problem, the problem is that it runs in the zone. One solution could be to register the event with regular js:

private showToolTip = false;

constructor(
  private ngZone: NgZone
) { }

myFunction(){
  ngZone.runOutsideAngular(() => {
      node.addEventListener('mouseover', () => {
        // compute the new state of the tooltip
        let new_showToolTip = ...
        // detect if the state changes
        if(new_showToolTip != this.showToolTip ){
          // if it does, run inside ngZone, this will trigger change detection
          ngZone.run(() => {
            this.showToolTip = new_showToolTip;
          })
        } // if not, it will not trigger change detection
      })
  })
}

I don't think there is a memory impact since no additional variables are used.

Even though the computation is a tiny bit more heavy, in the end you skip hundreds of change detection cycles.

Ploppy3 commented 5 years ago

@marjan-georgiev Tell me if you need more information!

4ctarus commented 5 years ago

up

sebastiangug commented 5 years ago

@Ploppy3 @4ctarus https://github.com/swimlane/ngx-charts/issues/1162#issuecomment-510093587

workaround -- I'll be making a pull request from ngx-charts-extra later this month, need to further test possible side-effects as it's a major change.