emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.44k stars 4.21k forks source link

[Bug] Memory leak with {{#each}} #19033

Open JackieChiles opened 4 years ago

JackieChiles commented 4 years ago

🐞 Describe the Bug

JS heap size grows substantially over time, indicating a memory leak, when updating an array rendered in an {{#each}} block in a component template. DOM nodes also increase seemingly without bound.

πŸ”¬ Minimal Reproduction

Click the "Run Tests" button in the Twiddle and observe JS heap size. Note that the Twiddle version of Ember is 3.18.1 as 3.19 doesn't appear to be supported yet, but I did try 3.17, 3.18.1, and 3.19 locally and they all show the same behavior. It also seems to affect both Glimmer and classic components.

https://ember-twiddle.com/5960c7f73dd7849064ebe43f22dbf4ed

πŸ˜• Actual Behavior

Over time, as an array bound to an {{#each}} block changes, JS heap size grows continuously. Manual garbage collection does not recover the allocated memory, nor does normal garbage collection when it occurs. The browser will eventually crash given large enough objects and enough time.

πŸ€” Expected Behavior

Expected heap size and number of DOM nodes to stop increasing.

🌍 Environment

βž• Additional Context

I did see https://github.com/emberjs/ember.js/issues/19020 but that ticket describes rendering components inside the loop. The example linked in this ticket does not render components. Also, the comments on that ticket indicate that the issue may have been resolved in Ember 3.19, but I can still reproduce with that version.

However, I cannot reproduce in 3.16.7, (to test you can just change the Ember version in the Twiddle), so it seems to have been introduced since then.

rwjblue commented 4 years ago

This was fixed in https://github.com/glimmerjs/glimmer-vm/pull/1113 (in glimmer-vm 0.54.1), and is already included in master branch (from https://github.com/emberjs/ember.js/pull/19040). I'll get that fix pulled into release branch for a future 3.20.1 patch release.

rwjblue commented 4 years ago

OK, pulled the changes back into beta (for release in 3.21.0-beta.2) and tagged + kicked off publishing of 3.20.1.

https://github.com/emberjs/ember.js/releases/tag/v3.20.1

@JackieChiles - I'll update here once that is published to NPM, so that you can test against it...

rwjblue commented 4 years ago

OK, release is out. @JackieChiles mind confirming things are resolved (my local tests seem good)?

JackieChiles commented 4 years ago

I will check and let you know.

steveszc commented 4 years ago

FWIW I was seeing this memory leak occur in a production app immediately after our upgrade to 3.16.0. Also, I am able to produce the memory leak in the reproduction twiddle on 3.16.5.

I can confirm that upgrading to 3.20.2 fixed the issue in our production app.

pzuraq commented 4 years ago

@steveszc that is strange, we wouldn't have expected the leak to show up until 3.17. I wonder if we had another leak that went undetected and was fixed by the VM upgrade..

steveszc commented 4 years ago

@pzuraq yeah that could be, the memory leak we were observing only really showed up on one of our most expensive routes. We were observing heap growth that was 2x pre3.16 and it seemed like GC was running less frequently and only major GC was actually freeing up memory. I have some memory profiles showing 3.16 vs 3.20 that I can post.

steveszc commented 4 years ago

Before - Ember@3.16.10 image

After - Ember@3.20.4 image