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

datepicker memory leak #17896

Closed
rudzikdawid opened this issue Dec 7, 2019 · 15 comments · Fixed by #18102, lingounet/testage#9, hrueger/AGLight#112 or hrueger/AGLight#114
Closed

datepicker memory leak #17896

rudzikdawid opened this issue Dec 7, 2019 · 15 comments · Fixed by #18102, lingounet/testage#9, hrueger/AGLight#112 or hrueger/AGLight#114
Assignees
Labels
animation This issue is related to Angular animations or CSS animations P2 The issue is important to a large percentage of users, with a workaround perf This issue is related to performance

Comments

@rudzikdawid
Copy link

Reproduction

Steps to reproduce:

  1. go to https://material.angular.io/components/datepicker/examples
  2. open datepicker and close multiple times
  3. each open and close action generate 135KB memory leak.

The more times we open and close datepicker, the more memory are allocated.
Garbage collector does not clear all allocated memory.

Expected Behavior

datepicker without memory leaks

Actual Behavior

memory leaks

Environment

  • Angular: 8.2.14, 9.0.0-rc.5
  • CDK/Material: 8.2.3, 9.0.0-rc.5
  • Browser(s): Chrome 78.0.3904.108
  • Operating System: Fedora 31
@rudzikdawid
Copy link
Author

related issue: angular/angular#25744

it looks like the biggest leak comes from @angular/animations module
after remove matDatepickerAnimations and rest of "animations" code from datepicker
each open and close action generate only 1.3KB of memory leak.

@Splaktar Splaktar added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful animation This issue is related to Angular animations or CSS animations perf This issue is related to performance labels Dec 13, 2019
@crisbeto
Copy link
Member

The animations leak is fixed by angular/angular#34409.

@jelbourn
Copy link
Member

Closing since this was caused by the framework and because the fix has been merged there.

@rudzikdawid
Copy link
Author

it's look like the memory leak still exists.
I built and launched dev-app from 9.0.0-rc.6 tag.
inside DOM the dev-app element has attibute ng-version="9.0.0-rc.7"
Also inside animations-browser.umd.js i see changes from fix angular/angular#34409.
So theoretically fix should work, but still each open and close action generate 130KB memory leak.

@mmalerba mmalerba reopened this Dec 28, 2019
@crisbeto
Copy link
Member

I can see the memory leak as well so the root cause probably wasn't angular/angular#34409. I spent some more time looking into it and I'm not sure exactly why it's happening, but here are some observations:

  1. It only happens for datepickers in popup mode. There's still a smaller leak in dialog mode, but it only ends up leaking one datepicker worth of elements and it doesn't stack up like the current leak.
  2. It seems to happen, because in popup mode we detach the overlay and we only dispose of it on destroy, but by that time the elements have leaked out already. If I switch to disposing it on every close, it starts behaving like dialog mode, but this isn't ideal because it breaks the closing animation.
  3. The leak seems to be due to the fact that we remove the DOM nodes once the animation has finished here. If I switch it to remove the element immediately, it stops leaking. This is confusing, because I can verify that the callback always fires at the proper time and the elements do get cleaned up. There might be something further down the line that expects the elements to have been removed immediately.

@crisbeto crisbeto self-assigned this Jan 4, 2020
@crisbeto crisbeto added has pr P2 The issue is important to a large percentage of users, with a workaround and removed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful labels Jan 4, 2020
crisbeto added a commit to crisbeto/material2 that referenced this issue Jan 4, 2020
Fixes the datepicker leaking memory when it is opened and closed in popup mode. The leak seems to come from the fact that we're detaching, rather than disposing of, the overlay when it is closed. We did this in the past to support an animation while closing the calendar, but it seems to cause the animations engine further down the line to retain the elements memory since they aren't being removed immediately.

These changes resolve the issue by triggering the exit animation, waiting for it to finish and disposing of the overlay. We weren't gaining too much by reusing the overlay from the previous approach, because the calendar was being removed and re-created every time anyway.

Fixes angular#17896.
mmalerba pushed a commit that referenced this issue Jan 7, 2020
Fixes the datepicker leaking memory when it is opened and closed in popup mode. The leak seems to come from the fact that we're detaching, rather than disposing of, the overlay when it is closed. We did this in the past to support an animation while closing the calendar, but it seems to cause the animations engine further down the line to retain the elements memory since they aren't being removed immediately.

These changes resolve the issue by triggering the exit animation, waiting for it to finish and disposing of the overlay. We weren't gaining too much by reusing the overlay from the previous approach, because the calendar was being removed and re-created every time anyway.

Fixes #17896.
crisbeto added a commit to crisbeto/material2 that referenced this issue Jan 10, 2020
Fixes the datepicker leaking memory when it is opened and closed in popup mode. The leak seems to come from the fact that we're detaching, rather than disposing of, the overlay when it is closed. We did this in the past to support an animation while closing the calendar, but it seems to cause the animations engine further down the line to retain the elements memory since they aren't being removed immediately.

These changes resolve the issue by triggering the exit animation, waiting for it to finish and disposing of the overlay. We weren't gaining too much by reusing the overlay from the previous approach, because the calendar was being removed and re-created every time anyway.

Fixes angular#17896.
mmalerba pushed a commit that referenced this issue Jan 10, 2020
Fixes the datepicker leaking memory when it is opened and closed in popup mode. The leak seems to come from the fact that we're detaching, rather than disposing of, the overlay when it is closed. We did this in the past to support an animation while closing the calendar, but it seems to cause the animations engine further down the line to retain the elements memory since they aren't being removed immediately.

These changes resolve the issue by triggering the exit animation, waiting for it to finish and disposing of the overlay. We weren't gaining too much by reusing the overlay from the previous approach, because the calendar was being removed and re-created every time anyway.

Fixes #17896.
@rudzikdawid
Copy link
Author

bad news: memory leak is a little bit smaller (120KB instead 130KB) but still exists.
I built and launched dev-app from 9.0.0-rc.8 tag.
Devtools Source panel, inside /src/material/datepicker/datepicker.js i see changes from #18102.

But still each open and close action generate ~120KB memory leak.

@crisbeto
Copy link
Member

The 40x leak is what I was referring to above when I said "There's still a smaller leak in dialog mode", but do you see the HTMLTableCell count increase if you keep opening and closing it? That's the part that my fix addressed.

@rudzikdawid
Copy link
Author

even if the leak comes from the dialog, calling it "small" is a lot of abuse.
This issue should be closed after the leak has been repaired.

@crisbeto
Copy link
Member

It's not small, but it's smaller since it doesn't leak for every open. I haven't been able to track down exactly where the other leak comes from, but it looks like it could be from the framework.

@crisbeto crisbeto reopened this Jan 17, 2020
yifange pushed a commit to yifange/components that referenced this issue Jan 30, 2020
Fixes the datepicker leaking memory when it is opened and closed in popup mode. The leak seems to come from the fact that we're detaching, rather than disposing of, the overlay when it is closed. We did this in the past to support an animation while closing the calendar, but it seems to cause the animations engine further down the line to retain the elements memory since they aren't being removed immediately.

These changes resolve the issue by triggering the exit animation, waiting for it to finish and disposing of the overlay. We weren't gaining too much by reusing the overlay from the previous approach, because the calendar was being removed and re-created every time anyway.

Fixes angular#17896.
@rudzikdawid
Copy link
Author

it looks like it's getting worse with every version of angular and components.

Angular 7.1.4
Components 7.3.7
Leak: 11.8 KB

Angular 8.2.10
Components 8.2.3
Leak: 59.5 KB

Angular 9.0.0-rc.13
Components 9.0.0-rc.9
Leak: 122 KB

@manklu
Copy link

manklu commented Feb 5, 2020

This may be part of the leak angular/angular#35148

@jelbourn
Copy link
Member

jelbourn commented Feb 6, 2020

I would also suspect something in the framework, since we haven't made any major changes to overlays or datepicker lately.

@rudzikdawid
Copy link
Author

angular@9.0.2 provides significant improvement in memory profile

@rudzikdawid
Copy link
Author

@angular/material 9.1.0solve memory leak in datepicker component

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
animation This issue is related to Angular animations or CSS animations P2 The issue is important to a large percentage of users, with a workaround perf This issue is related to performance
Projects
None yet
6 participants