Memory leak when using components

[giovannipiller]

Description:

Took me hours to pin this issue down in my SPA, but finally managed to reproduce it in a few lines of code 😪
Looks like Ractive 0.8 keeps pointing to unused DOM elements (and possibly something else? whole components?) after their component is teardown.

This may result in big memory leaks, depending on the size of your components.
The more the elements, the bigger the memory being leaked.

I started noticing it on components where 4k-20k elements were being removed. Narrowed it down to a minimal example.

Let me know if I can be of any help.

Versions affected:

0.8.11
notably, 0.7.3 is NOT affected

Platforms affected:

Any browser

Reproduction:

http://jsfiddle.net/trc4m077/4/

1. fire up the inspector (ex. Chrome)
2. activate inspector's Memory tab
3. click the button "TOGGLE ME", from my fiddle
4. use the Inspector to take a Heap Snapshot
5. filter objects by using the Class filter: "detached DOM Tree"

you should see 3 groups of Detached DOM Tree:

• ignore the first two, which are related to jsFiddle
• the third one should include 4 entries, all of which appear to be linked to Ractive

Those 4 entries will be a lot more, if you're building anything more complex than my example.

---

See also the attached screenshots from my inspector.
All screenshots have been taken after ApplicationHeader had been toggled off.

Code:

Interestingly, if you remove the {{name}}, no memory will be leaked.

From my tests, memory leaks even if ApplicationHeader is marked as isolated: true, and name gets passed via attribute.

const ApplicationHeader =  Ractive.extend({
  template: `
    <nav class="header">
      <section>
        My name is {{name}}
      </section>
    </nav>
  `,
});

const Application = Ractive.extend({
  template: `
  <button on-click="toggle('hideComponent')">TOGGLE ME</button>
  {{#if !hideComponent}}
    <ApplicationHeader/>
  {{/if}}
  `,

  components: {
    ApplicationHeader,
  },

  data: () => ({
    name: 'Laser',
  }),

});


window.r = new Application({ el: '#application' });

[fskreuz]

I think I encountered something similar last night while doing benchmarks but wasn't sure if it was Ractive or the benchmarks. Phantom kept eating up memory until it crashed. 🤔

[evs-chris]

@giovannipiller this is an issue with implicit mappings not being torn down fully. I think I've got it sorted now, but if you get a few minutes to try out v0.8-dev to verify that it's fixed, I'll publish 0.8.12 ASAP.

Thanks for the detailed report and enormous effort to track down the leak!

[evs-chris]

It looks like this is covered with v0.8-dev, but I'd still like to know that it covers more complex cases too: http://jsfiddle.net/trc4m077/5/

[giovannipiller]

I'll test it tomorrow (in ~10h) on my code and report back.

Thanks for the super quick turnaround 😊

[fskreuz]

Benchmarks I'm working on doesn't cause Phantom.js to explode anymore. Must be a good sign.

[giovannipiller]

Things definitely improved 🎉

Isolated another memory leak, very similar, but with computed properties accessing data from parent components.

http://jsfiddle.net/trc4m077/6/

Code for future reference

const ApplicationHeader =  Ractive.extend({
  template: `
    <nav class="header">
      <section>
        My name is {{altName}}
      </section>
    </nav>
  `,
  computed: {
    altName() {
      // no leak without this.parent
      return this.parent.get('name');
    },
  },
});

const Application = Ractive.extend({
  template: `
  <button on-click="toggle('hideComponent')">TOGGLE ME</button>
  {{#if !hideComponent}}
    <ApplicationHeader/>
  {{/if}}
  `,

  components: {
    ApplicationHeader,
  },

  data: () => ({
    name: 'Laser',
  }),

});


window.r = new Application({ el: '#application' });

[evs-chris]

@giovannipiller well the good news is that that one was already fixed on edge. It also made me think a bit about how template expressions and reference expression should be working. I found a few minor edge-case leaks there too, along with a few small bugs involving aliases and expression lifetime. All of that should be fixed on edge and v0.8-dev as soon as travis finishes building.

[giovannipiller]

Thanks Chris!

I'll report back this Monday morning (I'm from 🇮🇹, UTC+1), so I can test it in the most complex scenario at my disposal.

[paulocoghi]

(offtopic) @giovannipiller I'm going to Italy in April 14 and I'll stay at least 90 days to obtain my citizenship. Maybe we can exchange some ideas there in Italy. My skype username for future messages: "coghi.com.br"

[giovannipiller]

Leaks are getting harder to pin down.. which I guess it's a good sign 😃

Isolated another one here:
http://jsfiddle.net/trc4m077/9/

Code for future reference

const SubComponent =  Ractive.extend({
  template: `<footer>{{thisDoesntEvenExist}}</footer>`,
});

const ApplicationHeader = Ractive.extend({
  template: `
    <article class="header">
      Something here<br>
      {{#if tab}}
        <SubComponent/>
      {{/if}}
    </article>
  `,

  components: {
    SubComponent,
  },

  data: () => ({
    tab: true,
    anythingToObserve: 0,
  }),

  oninit() {
    /**
     * Observe will set a value different from the initial in data.
     * Note: { init: false } would "fix" this leak, but still..
     */
    this.observe('anythingToObserve', (newTab, oldTab) => {
      this.set('tab', null).then(() => {
        this.set('tab', true);
      });
    });
  },
});


const Application = Ractive.extend({
  template: `
  <button on-click="toggle('hideComponent')">TOGGLE ME</button>
  {{#if !hideComponent}}
    <ApplicationHeader/>
  {{/if}}
  `,

  components: {
    ApplicationHeader,
  },
});


window.r = new Application({ el: '#application' });

[giovannipiller]

Leak with transitions (0.8.12-build-4):
http://jsfiddle.net/wfdxgj55/5/

---

const Application = window.r = new Ractive({
  el: '#application',

  template: `
  {{#if !loaded}}
    <section class="if" fade-in-out>
      Wait for next transition
    </section>
  {{else}}
    <article class="else" fade-in-out>
      Transition completed. Watch out for a memory leak.
    </article>
  {{/if}}
  `,
  
  transitions: {
    fade(t, p) {
      // Any transition really, so keeping it short.
      // Tested with Fade and many other.
      t.complete()
    },
  },

  data: { loaded: false },

  onrender() {
    // example: this could by done after an AJAX
    this.set({ loaded: true });
  }
});

@evs-chris I'm keeping every new found leak under this issue, but I have no problem in creating multiple issues. Let me know if you have any preference, or need more data.

@paulocoghi drop me a line via email (see my profile)

[giovannipiller]

Might be worth noting that this leak doesn't occur on Edge: http://jsfiddle.net/trc4m077/12/

The latest one does though: http://jsfiddle.net/wfdxgj55/6/

Unfortunately I can't yet test my whole app on Edge, as it will take some time to cover all the deprecations.

Last update for today: I noticed at least a couple more leaks occurring. Here are a few screenshots from my Heap Snapshots. Will try to isolate them tomorrow.

[evs-chris]

I found part of the unresolved leak in 0.8, but part of it has eluded me so far. Again, the good news, as you've discovered, since 0.9 dropped unresolved support, all of the resolver related leaks don't affect it.

[evs-chris]

I think I've got the second part of that unresolved leak covered now. The transition leak at least wouldn't compound, but it should also be handled now too. v0.8-dev should be 0.8.12-build-6 and edge should be 0.9.0-build-110.

Based on those reference traces, the last unresolved fixes may cover those as well.

I'm good with running out the rest of the leaks in this issue. Thanks for isolating them!

[giovannipiller]

The great news is that the number of detached trees in my app went down dramatically 😃
Tested on a page that used to have 24k of them, and you can definitely feel the difference!

I'll still investigate the very few other leaks I'm experiencing, as they might be a big deal for other people.

Things are beginning to work quite nicely 😎 🎉

---

Isolated the EventDirective leak (biggest offender now):
http://jsfiddle.net/7w5sq8ad/3/

---

const Application = Ractive.extend({
  template: `
  {{#each stuff}}
  	<a href="#anything" on-click="@this.fire('myEvent')">Link</a>
  {{/each}}
  `,
  
  data: {
  	stuff: [1,2,3],
  },
  
  onrender() {
  	// could be setting the array to [] rather than [1,2,3,4], the result is the same
  	this.set('stuff', [1,2,3,4]).then(() => {
    	this.set('stuff', [1,2,3]);
    });
  }
});


window.r = new Application({ el: '#application' });

Notably, no leak when using the deprecated bindings:

<a href="#anything" on-click="myEvent">Link</a>

[giovannipiller]

Found another one, though it might be related to my previous example:
http://jsfiddle.net/14ga8u50/1/

---

const ApplicationHeader = Ractive.extend({
  template: `<article>Hi, I'm a component</article>`
});

const Application = window.r = new Ractive({
  el: '#application',

  template: `
  {{#if stuff}}
  	<ApplicationHeader something="{{thatDoesntExist()}}"/>
  {{/if}}
  `,

  components: {
    ApplicationHeader,
  },

  data: {
    stuff: true,
  },

  onrender() {
    this.set('stuff', false).then(() => {
      this.set('stuff', true);
    });
  }
});

---

I also noticed another one (kinda big in my app) possibly related to computed properties, but likely won't be able to isolate it by today. Never mind, that was probably my fault. Will double check tomorrow.

So, to sum up: I still need to isolate at least 2 (actually 1) more scenarios.

But still, today is a good day :)

[evs-chris]

The event directive leak wasn't an issue for edge, but I've got it patched up in v0.8-dev now. The mapping leak is also patched for both edge and v0.8-dev. Thanks!

[giovannipiller]

Leak occurring with {{#each}} and data changing quickly:
http://jsfiddle.net/etapo71q/3/

---

const Application = window.r = new Ractive({
    el: '#application',

    template: `
  <section>
    {{#each stuff}}
        Something to display.<br>
      {{else}}
        <div>
          {{#if doesntEvenExist}}
            AAAAA
          {{else}}
            BBBBB
          {{/if}}
        </div>
      {{/each}}
  </section>
  `,

    data: {
        stuff: null,
    },

    onrender() {
        this.set('stuff', []).then(() => {
            this.set('stuff', [1, 2, 3]);
        });
    }
});

---

Still at least another one to isolate.

[giovannipiller]

Still at least another one to isolate.

False alarm. These were all issues on my end.
Just this one left.

Nothing else to report for now .... 😎 🎉

[evs-chris]

I think I got that last one sorted out on v0.8-dev (0.8.12-build-8). It's resolver-related, so it didn't affect edge 🎉

[giovannipiller]

Hate to say it, but I've found a couple more :(

Not sure if I'll be able to isolate them by today, so in the meantime I'm attaching the reference traces to give you an idea.
Worst case scenario, I'll work on it this Monday.

---

This won't be super easy to isolate, as it's in the middle of a component with decorators performing dark magic (aka generating charts).

Don't be too afraid by the number of trees/entries. It's an SVG.

---

This one is seems much easier to pin down. I'll see what I can do today!

[mefjuu]

Offtopic: could you explain shortly how to read such snapshots to find memory leaks?

[giovannipiller]

@mefjuu a great post on the matter is: https://developers.google.com/web/tools/chrome-devtools/memory-problems/

Specifically the section: Discover detached DOM tree memory leaks with Heap Snapshots

[giovannipiller]

Unable to reproduce the 2nd issue with plain Ractive. Must be an small, crazy issue, with my bundler/transpiler/whoknows 😭.

Pasting here the fiddle for (my) future reference. This exact code results in leaks on my end, basically nothing else except Webpack and transpiler are involved.
http://jsfiddle.net/9htevrj3/1/

This one is seems much easier to pin down

Yeah, right

[evs-chris]

Based on those heap dumps, I took a guess at what might be causing both of those last leaks. The 0.8 version should be build-9. We're having a little trouble with travis publishing edge, but when that gets sorted, the same change should be available on edge.

[giovannipiller]

I can confirm you that the 2nd issue is definitely fixed now. Awesome 😄 🎉

Tried to pin down this one today, but had very little time so nothing to report yet. Still not fixed though.
Will try again tomorrow.

[evs-chris]

I found one more potentially leaky place. Does there happen to be a series of splice operations involved with that first leak (push, pop, shift, unshift, splice, sort)?

[giovannipiller]

Does there happen to be a series of splice operations involved with that first leak (push, pop, shift, unshift, splice, sort)?

Oh yes!
We're building SVG charts based on data from multiple sources, so a decent number of arrays' operations are involved.

[evs-chris]

I took a swing at a potential issue involving multiple rebinds (splices) in one turn of the runloop that may resolve that first issue. Since it's an issue with unresolveds, it shouldn't affect edge. The 0.8 build with the change is 0.8.12-build-10.

[giovannipiller]

Identified the first issue. It was quite simple actually:
http://jsfiddle.net/x0astm5z/3/

---

---

const ApplicationHeader = Ractive.extend({
  template: `
  <article>
    test test
  </article>
  `,
  onrender() {
    // this will leak memory
    this.observe('unexistentStuff', () => {})
  }
});


const app = new Ractive({
  el: '#application',

  components: {
    ApplicationHeader,
  },

  data: {
    show: true,
  },

  template: `
  <main>
    {{#if show}}
      <ApplicationHeader/>
    {{/if}}
  </main>
  `,

  onrender() {
    this.set('show', false);
  },
});

[evs-chris]

This one is slightly trickier. What's happening is the onrender for the component is set to fire at the end of the event loop, but the onrender for the main instance is causing the component to be torn down. When the call to observe actually runs, the component is already torn down, so it never gets a chance to be torn down again.

I see a few ways to handle this:

1. Throw or just ignore observe calls on torndown instances. Ignore sounds like it would be safe because a torndown instance should never have an opportunity to fire.
2. Don't call the render hook if the instance has already been torndown, but that may have strange lifecycle implications.
3. Leave it to the user to handle this as an edge case. (observing an unresolved keypath from the render hook of a component that gets torn down before it finishes rendering)

I think option 1 is probably the way to go because it is the least likely to introduce breakage and confusion. There may be a few other methods that need to check for torndown too, like link. on may be a potential candidate for a check too, but I don't think it would have any detrimental effects to set up a listener on a torn down instance. link doesn't do unresolveds, so it may be relatively safe too.

[evs-chris]

I went ahead with option 1, which is currently being built by travis to be published as build-11. I didn't add the same check to edge, since there are no unresolveds to cross component boundaries and hold onto torn down instances.

[giovannipiller]

Agreed, option 1 seems the best.

Will test again tomorrow and report back. Thanks!

[giovannipiller]

Looks like build-11 failed to be published https://travis-ci.org/ractivejs/ractive/jobs/211115330

[evs-chris]

Ah! There was an old deploy script on the 0.8 branch. I've updated it, and build-11 is now published.

[giovannipiller]

Confirmed as fixed. Tested build-11 it quite a bit today, and found nothing new.

As far as I can tell, I think we're done here 🎉 🎉 🎉 😄
I'll keep an eye on memory consumption from now on, and open new issues if necessary.

Again, thank you so much @evs-chris for the massive help!
0.8.12 is going to be a pretty awesome minor release! 😎

[evs-chris]

Thanks for doing the tracking! I've pushed out 0.8.12 🎉

[wearhere]

Awesome work folks. We @mixmaxhq used to use Meteor i.e. Blaze for our frontend, then switched to Backbone due to performance problems, but are now considering Ractive to simplify some complex UIs, and it's terrific to see that there's such a great community actively improving it.

[fskreuz]

Meteor 0.6... those were the days. 😄