-
Notifications
You must be signed in to change notification settings - Fork 377
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
base: master
Are you sure you want to change the base?
Conversation
@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 |
@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.
@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. |
@steveszc revisiting this now. Would you possibly be able to rebase this PR? |
@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. |
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 viaember 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.@triggerComponent
receives the@onFocus
action that triggers it