search

IgniteUI / igniteui-angular

Ignite UI for Angular is a complete library of Angular-native, Material-based Angular UI components with the fastest grids and charts, Pivot Grid, Dock Manager, Hierarchical Grid, and more.
https://www.infragistics.com/products/ignite-ui-angular
Other
568 stars 159 forks source link

igx-grid memory leak when users moved columns #13993

Open jsakamotoIGJP opened 3 months ago

jsakamotoIGJP commented 3 months ago

Description

The igx-grid has a memory leaking problem. It happens when users move columns.

Steps to reproduce

  1. Open the browser's Dev Tools / Memory tab.
  2. Take the 1st memory snapshot (don't forget to click the "Collect garbage" button before taking.)
  3. Navigate to the page that has an igx-grid component.
  4. Move columns of the igx-grid by drag & drop a column.
  5. Go back to the home page.
  6. Take the 2nd memory snapshot (remember to click the "Collect garbage" button before taking it).
  7. In the Dev Tools Memory tab, Choose "Comparison" in the "Perspective" dropdown and input "igx" into the "Class filter" text field.

https://github.com/IgniteUI/igniteui-angular/assets/102948935/a55c1f6a-2ea5-4429-9b3e-57c261ca333f

Result

You will see there are some objects with the name "igx" that were added but not deleted in the "Comparison" view of the Memory tab.

Expected result

The "Delta" column for all objects with the name "igx" should be zero in the "Comparison" view of the Memory tab.

Attachments

📦c-00235192-ng-app1-v17.1.4.zip

ChronosSF commented 2 months ago

@jsakamotoIGJP , we'll investigate this before end of April. Please, have in mind that Angular caches templates for quicker rendering. GC in browsers are also not triggered consistently based on what you are doing.

Also for this to even affect the user, he needs to trigger it (move columns in this case) consistently over a lot of time which is not a real-life use case for column moving.

ChronosSF commented 2 months ago

@jsakamotoIGJP , I took a look into this today I would first want to say that the way of identifying a memory leak is not what you describe in the steps to reproduce. In addition going between routes triggers all kinds of cachings so getting any meaningful result in the memory snapshots is almost impossible. You also introduce the angular router into the equation so now the cause of the memory leak is no longer certain as angular router has been known to have leaks too.

What you do to identify these leaks is something like this:

  1. You suspect say moving columns cause a leak
  2. You move a column once.
  3. You trigger the browser garbage collection.
  4. You take a snapshot.

You do this a few times and try to compare snapshots. I did see that after each move the heap does increase with around 1kb of stuff, however, I am so far unable to pin down the exact culprit for it. To be honest in this specific case even if we can't get to the bottom of it, users will never experience performance deterioration from such a leak as first the leak is minuscular and second it's triggered by a user interaction that is rare in any real-life scenario.

We'll keep looking into this but if the cause escapes us for some more time, investing more into researching this may start being counterproductive.

jsakamotoIGJP commented 2 months ago

Excuse me, but I cannot agree with all of your replies.

the way of identifying a memory leak is not what you describe in the steps to reproduce.

If so, could you explain the correct way to identify a memory leak? I focused on the number of specific types of objects that remain in the browser memory, such as "_IgxGridComponent", not the total amount of heap memory size. To be honest, I'm not very confident in this technical area. I'd appreciate it if you could only provide some hints or links.

You also introduce the angular router into the equation so now the cause of the memory leak is no longer certain as angular router has been known to have leaks too.

But you've resolved other memory leaks, haven't you? (See also the isue: https://github.com/IgniteUI/igniteui-angular/issues/13923) I'm wondering why you want to make this case exclusive.

it's triggered by a user interaction that is rare in any real-life scenario.

OK, you are right. I partially agree with you at this point. However, please keep in mind that there is a kind of Angular app that runs unbelievably long (almost a few months) on limited resources, like 2GB memory single-board computers. In fact, I was working on developing such an Angular app for a manufacturing factory a few years ago. We might not have to spend time resolving the case of the column moving because it is too rare to fix, as you said, but the case of the pagination (see: https://github.com/IgniteUI/igniteui-angular/issues/13994) sometimes causes a critical problem in that "factory" scenario. Therefore, I would appreciate it if you would prioritize issue #13994 rather than this issue.

Anyway, if the team decides to leave this issue unsolved due to team resources or time frames, I respect that decision. I feel it is not ideal, but I know that we need to accept some trade-offs.

ChronosSF commented 2 months ago

Ideally, when searching for a leak you want to do a single repeatable action and take snapshots between executing the said action. You also have to consider the browser garbage collection which doesn't trigger consistently so you always want to trigger it manually before taking the snapshots.

This is why I suggested that if you just move columns you do see a leak, it's just not severe. Using the router adds noise and the router itself is prone to leaks of its own. It's much better to setup an ngIf switch if you want to test for leaks after component destroy.

That being said I am not excluding the possibility of leaks after destroy and if and once we fix the issue with moving columns we'll also look into the possibility of a destroy leak.

I also agree that the paging leak, if present, is a more severe issue as the action that causes the leak is supposed to repeated often. That one we'll address regardless of how big the leak is.

On a side note, I do miss the old Edge (before they adopted chromium) as it had the best memory profile by far. With all the modern ones it's a huge guessing game.