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

Fix memory leaks / Add ember-cli-memory-leak-detector #1430

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

steveszc
Copy link

@steveszc steveszc commented Feb 15, 2021

This PR installs an add-on I authored called ember-cli-memory-leak-detector.

The add-on works by recording a heap snapshot after the suite tests finishes running. The heap snapshot is then searched for any retained instances of the host app/addon's ES classes (indicating a memory leak). If it finds retained classes in the heap snapshot, the test suite fails and a report of the retained classes is logged in the console output.

After installing ember-cli-memory-leak-detector (and fixing the few remaining leaky codepaths) we can ensure that no memory leak is ever introduced into EPS in the future, which is a big win for a such a popular addon!

I'll be giving a talk at EmberConf this year about ember-cli-memory-leak-detector and I'd like to get it installed in at least one or two popular addons prior to giving the talk. EPS was a great candidate because it is popular, already using ES classes, and the surface area of existing memory leaks seems limited. If ember-cli-memory-leak-detector gets traction from ember conf we can move the ember community toward eliminating memory leaks altogether!

To-Do

ember-cli-memory-leak-detector identified a few memory leaks that I haven't been able to fix on my own. I have narrowed down a few individual tests that exercise leaky code paths, however it's unclear whether it is the test itself that is leaky or the code that is being exercised that is leaking.

One common theme I noticed is that tests that exercise touch events on options seemed to be causing leaks. I did find that in a few of these tests, just triggering the touchstart event on an option was enough to cause the PowerSelect class to be retained, but I couldn't identify any obvious leaks in the event handlers for touch events.

These tests can be isolated either via ember test --filter="..." or via ember test --server and manually filtering in the browser test page, and ember-cli-memory-leak-detector will either fail or pass individual tests based on the heap snapshot generated after running the individual test.

  • Integration | Component | Ember Power Select (Customization using components): the @triggerComponent receives the @onFocus action that triggers it
  • Integration | Component | Ember Power Select (Touch control): Touch on option should select it
  • Integration | Component | Ember Power Select (Touch control): Touch on custom option should select it

@cibernox
Copy link
Owner

cibernox commented Mar 6, 2021

@steveszc sorry for delaying this so much, but finally I was able to find time to move CI to github actions and fix it.

Can you rebase, this time it should be green

@cibernox
Copy link
Owner

@steveszc ping

ember-cli-memory-leak-detector identified 3 leaked instances of
PowerSelect after a full run of the test suite.
Converting 3 instances of onclick to {{on "click ...}} in test code
fixed the leaks that were found.
@steveszc
Copy link
Author

@cibernox all rebased, however it looks like there are a couple memory leaks still. I've updated the PR description with the 3 tests I found that appear to be leaking the PowerSelect class. Haven't had time to dive into why they are leaking.

@RobbieTheWagner
Copy link
Collaborator

@steveszc revisiting this now. Would you possibly be able to rebase this PR?

@steveszc
Copy link
Author

steveszc commented Aug 3, 2022

@rwwagner90 Until this issue gets fixed it is unlikely that ember-cli-memory-leak-detector will run successfully with the newer versions of Qunit. I haven't had a chance to work on it, sadly.
steveszc/ember-cli-memory-leak-detector#49

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

Successfully merging this pull request may close these issues.

3 participants