4

I have a very stupid performance issue.

I have a component that uses ngStyle, and I'd prefer to not rewrite it. But each time I click random input on the same page (even from another component), ngStyle recalculates (and do it pretty slow).

Let's say I want to have a table of multiply with dynamic background:

<section>
  <div class="row" 
       *ngFor="let row of rows">
    <div class="col" 
         [ngStyle]="{'background-color': getBG(row*col)}" 
         *ngFor="let col of cols ">
      {{row * col}}
    </div>
  </div>
</section>

Then on the same page I want to add several inputs for some reason:

<section>
  <input type="text" [ngModel]="model1"/>
  <input type="text"[ngModel]="model2"/>
  <input type="text"[ngModel]="model3"/>
  <input type="text"[ngModel]="model4"/>
  <input type="text"[ngModel]="model5"/>
</section>

Now each time I click on one of those inputs - getBG() will be called. And even if that function just returns a string without any calculations - it's still super slow

Example at StackBlitz - just open console and try to click swiftly among different input fields or enter a value. Even as a user I can see it's not responsive at all


UPD1: My real-world case is much more complex. And already use ChangeDetectionStrategy.OnPush. Binding ngStyle to a value instead of function also doesn't help much - it faster but still slow (and produces a lot of complexity). What I want, it's probably a way to tell ngStyle to not recalculate until I'll ask explicitly. Maybe ChangeDetectorRef.detach()could help

Community
  • 1
  • 1
S Panfilov
  • 16,641
  • 17
  • 74
  • 96
  • maybe you'd better avoid the function and map the bg colors into a matrix array (or an object with keys [row +'-'+col]) – G-Host Jul 16 '19 at 15:08
  • This might help you : [Angular : escape from change detection](https://netbasal.com/angular-2-escape-from-change-detection-317b3b44906b) – Jeremy Thille Jul 16 '19 at 15:19
  • 1
    Function calls in template are universal performance killers. Don’t do it, run the function in your component once and assign it as a property on your columns and assign it that way. – bryan60 Jul 16 '19 at 15:31

2 Answers2

8

That makes perfect sense. This is how Angular performs change detection. And this is Angular performing extra checks since you called a function in one of the data-binding syntaxes, here:

[ngStyle]="{'background-color': getBG(row*col)}" 

Angular performs Change Detection in three cases:

  1. DOM Events.
  2. AJAX Calls.
  3. Timeouts / Intervals.

This is the case of DOM Events (click).

Now when performing Change Detection, Angular check whether a particular variable in the Component has changed.

That's pretty straight forward in case of properties. But not so straight-forward in case of functions.

You see, the only way to determine whether the value of a function has changed is by calling it.

So Angular is doing just that.

SOLUTION:

Just create a matrix for the number to show and the color to paint right in the Component Class:

import { Component } from '@angular/core';

@Component({
  selector: 'my-app',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.css']
})
export class AppComponent {
  rows = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
  cols = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
  matrix = [];

  model1 = '';
  model2 = '';
  model3 = '';
  model4 = '';
  model5 = '';

  ngOnInit() {
    this.rows.forEach((row, rowIndex) => {
      this.matrix.push([]);
      this.cols.forEach((col, colIndex) => {
        const product = row * col;
        this.matrix[row].push({
          numberToShow: product,
          color: this.getBG(product),
        });
      })
    });
  }

  getBG(hue: number): string {
    console.log('getBG was called');
    return 'hsl(' + hue + ', 100%, 50%)';
  }

}

And then use it in your template:

<br/>
<div> 1. Open a console</div>
<br/>

<section>
    <div class="row" *ngFor="let row of matrix">
        <div 
      class="col" 
      [style.background-color]="col.color" 
      *ngFor="let col of row ">
            {{col.numberToShow}}
        </div>
    </div>
</section>

<br/>
<div>2. Click fast on the different inputs: </div>
<br/>

<section>
    <input type="text" [ngModel]="model1"/>
  <input type="text"[ngModel]="model2"/>
  <input type="text"[ngModel]="model3"/>
  <input type="text"[ngModel]="model4"/>
  <input type="text"[ngModel]="model5"/>
</section>

Difference in the performance:

In the previous implementation, the getBG was called 401 times on initialization.

In the solution implementation, the getBG is called 101 times on initialization.

That's a massive performance gain of around 397%.

Plus there's no extra call to the getBG method when the user focuses and blurs out from any input field.

Here's a Working Sample StackBlitz for your ref.

You might also want to read through a Medium Article that I wrote about Performant Reactive Forms in Angular. Although it's related to Reactive Forms, I've touched upon this aspect, in the article. I'm sure you'll find it helpful.

Community
  • 1
  • 1
SiddAjmera
  • 38,129
  • 5
  • 72
  • 110
  • It's interesting and explains the why, but it doesn't give a solution as to how to prevent it. – Jeremy Thille Jul 16 '19 at 15:17
  • @JeremyThille, ahh, I'll update the answer for that. Sorry about that. :) – SiddAjmera Jul 16 '19 at 15:18
  • 1
    Angular performs change detection generally for asynchronous events and DOM events, a promise will also run change detection along with the async pipe and a few others – bryan60 Jul 16 '19 at 15:29
  • Honestly that's not a solution at all. It's a demo example and it works, but in a real world app I still have a huge performance degradation if I use ngStyle even with a single static value. I'd prefer to say Angular "dude, don't recalculate until I ask explicitly" – S Panfilov Jul 17 '19 at 07:56
  • @SergeiPanfilov, unfortunately, there's no *one-size-fits-all* solution to this issue. There may be different solutions to different types of issues and use-cases. I tried to answer based on what was given at hand. There has been a series of similar questions asked in this regard. [One was asked today as well](https://stackoverflow.com/a/57069496/2622292). But like I said, the solution would significantly depend of that specific use case. I'd still suggest you to look out for a *one-size-fits-all* solution. If you do come across one, please do add it as an answer on this thread. Good Luck :) – SiddAjmera Jul 17 '19 at 08:47
  • @SiddAjmera Okay, fair enough. I'll mark your answer as an "accepted", cause I guess it will helps most of the people who will see this question – S Panfilov Jul 17 '19 at 10:44
2

The detection is slow for 2 reasons. Dev tools are kinda slow and printing many messages can show things even more.

The other point is, that you are doing unnecessary work. By separating the two parts you will be able to change the changeDetection strategy to OnPush.


Simplified example:

@Component({
    selector: 'my-cell',
    template: '<div [ngStyle]="styles"><ng-content></ng-content></div>',
    changeDetection: ChangeDetectionStrategy.OnPush,
})
export class CellComponent {
    @Input() styles: {
    readonly "background-color": string;
  };
}

and

@Component({
    selector: 'my-app',
    templateUrl: './app.component.html',
    styleUrls: ['./app.component.css']
})
export class AppComponent {
    rows = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
    cols = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
    matrix: {
        numberToShow: number;
        styles: {
            readonly "background-color": string;
        };
    }[][] = [];

    model1 = '';
    model2 = '';
    model3 = '';
    model4 = '';
    model5 = '';

    ngOnInit() {
        this.rows.forEach((row, rowIndex) => {
            this.matrix.push([]);
            this.cols.forEach((col, colIndex) => {
                const product = row * col;
                const self = this;
                this.matrix[row].push({
                    numberToShow: product,
                    styles: {
                        get "background-color"() {
                            console.log('background-color read');
                            return self.getBG(product)
                        },
                    },
                });
            })
        });
    }

    getBG(hue: number): string {
        return 'hsl(' + hue + ', 100%, 50%)';
    }
}

<section>
    <div class="row" *ngFor="let row of matrix">
        <my-cell [styles]="col.styles" *ngFor="let col of row">
            {{col.numberToShow}}
        </my-cell>
    </div>
</section>

<section>
    <input type="text" [ngModel]="model1"/>
    <input type="text" [ngModel]="model2"/>
    <input type="text" [ngModel]="model3"/>
    <input type="text" [ngModel]="model4"/>
    <input type="text" [ngModel]="model5"/>
</section>

The OnPush detection strategy means that, if any of @Inputs of given Component/Directive changes, it will trigger do detection. If you want to make use of that, you have to separate the expensive part into a separate directive, while making sure, its @Inputs will only change if necessary.


StackBlitz: https://stackblitz.com/edit/style-performance-of-a-grid-fzbzkz

Akxe
  • 9,694
  • 3
  • 36
  • 71
  • It sounds promising, I'm interested in this answer, but IMO you explain too few. What is `ChangeDetectionStrategy.OnPush`, how exactly does it solve the problem, how exactly do you `check if 'generatedTable' needs to be recalculated`? – Jeremy Thille Jul 17 '19 at 07:07
  • `OnPush` won't help here at all - cause `OnPush` means to detect changes after user interact with inputs. Check the updated example - each click still produces recalculations. – S Panfilov Jul 17 '19 at 08:07
  • @JeremyThille Updated, see it now – Akxe Jul 17 '19 at 20:32