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

Memory leaking #1157

Closed
frozeman opened this issue Jun 15, 2013 · 45 comments
Closed

Memory leaking #1157

frozeman opened this issue Jun 15, 2013 · 45 comments

Comments

@frozeman
Copy link

hi,

postet this already in the meteor-core group, but after testing more i guess this belongs here.

I figured, that when switching templates really often it stacks up memory. Which shouldn't be the case, because for large apps this can be fast at 100MB+ after a while.

Test case:

install the todos example of meteor $ meteor create --example todos

open the chrome dev tools and take a memory snapshot under "profiles".

then switch the todos lists like 20 times and takt one again and you will see an increase in the memory.

If you want to have a more obvious example got to the telescope demo app at http://demo.telesc.pe and do the same setps but switch the list with a lot of comments back and forth.

you will build up 50MB+ mb really fast, starting at 10MB!

This is obvious bad memeory management of the template engine and can lead to crashing apps in production environment with apps which are used for hours.

Thanks for looking into it.
Fabian

@frozeman
Copy link
Author

btw, the same happens for the meteor.com site, when only switching between 'home' and 'examples'.
i tested the same on an ember.js app and the memory gets always properly cleared again.

@awatson1978
Copy link
Contributor

YES!
I just posted something similar in the Google Talk group! Are you finding it crashes the Chrome Dev Tools themselves? After a bit of switching back and forth, I'm finding that the Dev Tools themselves start getting sluggish and eventually crash. Something has gotten leaky, unfortunately. :(

@frozeman
Copy link
Author

no it doesnt crashes chromes dev tools for me, but stacking up al lot of memory, the more i use the website.
this is certainly critical for me, as i want to develop a quite large app with meteor and if this issue is not resolved i hav to choose another framework (which i would like to prevent :)

@frozeman
Copy link
Author

A Blog post which could help. solving this problem:
http://coding.smashingmagazine.com/2012/11/05/writing-fast-memory-efficient-javascript/

@awatson1978
Copy link
Contributor

Mmm. Yeah, I already adopted all those functional patterns into my coding style a few years ago. Never use 'new', no timers, avoid 'var' as much as possible, etc. Good practices though! :)

So, there's a related discussion happening in the google talk meteor list...
https://groups.google.com/forum/m/?fromgroups#!topic/meteor-talk/49q2JcAxgMA

First of all, check whether you're using Meteor.autosubscribe, whic is depreciated, instead of Meteor.autorun. It's looking like use of a depreciated auto publish may be a common factor of people having memory leaks.
http://www.meteor.com/blog/2013/02/13/meteor-055-devshop-code-and-community-contributions

If that doesn't fix things, lets start taking a close look at the changes made in 0.5.7. They did a lot of tinkering with how the Spark engine observes live-data updates, and if this is is a real memory leak (and not simply the result of using a depreciated function) it may have been introduced around then.
http://www.meteor.com/blog/2013/02/21/meteor-057-major-scaling-update-new-ddp-version-ejson

@frozeman
Copy link
Author

Could help too:
http://code.google.com/p/leak-finder-for-javascript/

I use only collection and template., no subsciptions at all, as i only want to use the frontend part of meteor

@awatson1978
Copy link
Contributor

Oh my. That there is some voodoo magic, aint it? But it might do the trick.

I guess maybe set goog.Disposable.MONITORING_MODE=goog.Disposable.MonitoringMode.INTERACTIVE in the Meteor.startup()?

@frozeman
Copy link
Author

It add it says:
Uncaught ReferenceError: goog is not defined

@awatson1978
Copy link
Contributor

Well, I almost got things working. I resolved the ReferenceError by copying the following files into my Meteor project:

// original files
// http://closure-library.googlecode.com/svn/trunk/closure/goog/disposable/disposable.js
// http://closure-library.googlecode.com/svn/trunk/closure/goog/disposable/idisposable.js
// http://closure-library.googlecode.com/svn/trunk/closure/goog/base.js

// file structure
client/compatibility/disposable.js
client/compatibility/core/idisposable.js
client/compatibility/core/base.js

Then hiding the following two files, so Meteor didn't parse them, like so:

/leak-finder/.doc
/pyautolib/.dom_mutation_observer.js

And adding the following to my application:

Meteor.startup(function(){
    goog.require('goog.Disposable');
    goog.Disposable.ENABLE_MONITORING = true;
    goog.Disposable.MONITORING_MODE = goog.Disposable.MonitoringMode.INTERACTIVE
});

At which point, after relaunching the Meteor server, and refreshing the page displaying the app, when I check the inspection on localhost:9222, the page I'm testing for a memory leak is sort of grayed out, and I'm getting the following message in the console:

INFO:root:Reading suppressions from "closure-disposable-suppressions.txt"
INFO:root:Taking heap snapshot

But then it just hangs there. Any ideas?

@frozeman
Copy link
Author

Why you don't use the chrome dev tools timeline -> memory observer? Or snapshot?

You also can then observe objects etc.

And why you think the memory thing changed in 5.7?

I assume that the templates aren't disposed correctly and still hang around in the memory.

@frozeman
Copy link
Author

Any progress here?
I tested it with ember.js, and filling up views and replacing them doesn't stack up memory like spark.

here the memory gets cleaned up properly so that my app never goes beyond 6mb, only for short periods when loading a heavy template.

So i guess, there is definitively something wrong with spark, or the way the template lifecycle works.

@frozeman
Copy link
Author

Is there anyone working on this issue? As it seems rather important for app stability.

@avital
Copy link
Contributor

avital commented Jun 20, 2013

@frozeman I'm looking into this.

@avital
Copy link
Contributor

avital commented Jun 20, 2013

I loaded the todos app (on Meteor 0.6.4). I used Chrome Dev Console to take a heap snapshot right after pageload and found 4.4MB.

I then ran the following Javascript for a few minutes, which switches back and forth between different lists (which as I understand is the repro suggested by @frozeman)

setInterval(function () {
  Router.setList(Lists.find().fetch()[0]._id);
  setTimeout(function () {
    Router.setList(Lists.find().fetch()[1]._id)
  }, 150);
}, 300);

Notably, I do see the lists changing back and forth and all items in the list appearing each time. I then took a heap snapshot once every minute or so as this code was running. Heap size grew to around 5.5MB after which it stopped growing.

I believe this demonstrates that there is no memory leak (though it may be possible to consume less memory in general). Am I running a bad test?

@frozeman
Copy link
Author

can you try this again using the "timeline" pane in chrome dev tools (swtch the memeory recoding buttons on)?

i tried my own app, i did a small test with the todo list app, but not long.
the most obvious it is in the telescope app.

What i did for testing, is to create a template with an {{#each}} over a cursor. Then i fill in that collection like 1000 objects, and switch the view back and forth to and from that list. Son it rises up to 50MB+ and never goes down, even if the template is not visible. and even if i press the "trashcan" button, to clean up the garbage collection in chrome dev > timeline.

I used the technic described below, but also used plain {{> templateWithLargeList}} helpers to test it with the same result.

the technic i used to dynamically create templates.
when i set template dynamically like this:

  // setting a session variable to trigger the right template and then i load it dynamically
  Session.set('myTemplate','someTemplate');

  Template.myContainerTemplate.showTemplate = function() {
   return Template[Session.get('myTemplate')]();
  }

Does this create always new instances of that template, which never gets cleaned up? And if so, how to properly clean up that template, as i find this solution for dynamically setting templates, very nice.

I also did the same setup in ember.js and it grows memeory but always after a few sec. clean it up and it basically never goes beyond 6mb, even if i had big lists...

@frozeman
Copy link
Author

Here is a screenshot, and as you can see it grows rather big.
I used a collection, which i filled with 100 items and then cleared it by remiving each item by id and the fill it again, when the ratings link is clicked and they are shown... and then i click back and forth a few times..

screen shot 2013-06-21 at 09 03 49
screen shot 2013-06-21 at 09 06 31

@avital
Copy link
Contributor

avital commented Jun 21, 2013

Wow, I hadn't known about this 'Timeline' feature! Thanks for suggesting it. Here's the graph I get (notably, I never turned off the timer so it is constantly switching back and forth).

As I said before, this explicitly shows that switching back and forth between lists does not introduce a memory leak, since memory consumption does not continue to grow (your graph suggests the same). Whether Meteor is wasteful in memory usage or not is a completely different issue.

screen shot 2013-06-21 at 10 06 58 am

@frozeman
Copy link
Author

What i see is that it grows, it goes down once in a while, but for me it grows if i would click more back and forth. so in the long run the memory increases.

It seems that there is a clean up, but a part of the memory stays and stacks up, with time.

Do you have any suggestion because of the dynamically loading of templates, is this a good practice like this? or does it not get cleaned up properly..?

btw. i also just found that timeline feature, its pretty neat :)

i'm building a large app in meteor and don't want to run into issues in the long run.

@avital
Copy link
Contributor

avital commented Jun 21, 2013

@frozeman I have not yet seen any evidence supporting your claims. Both your and my experiment show memory growing and stabilizing around some upper limit. Can you show me a timeline graph that actually does continue growing without bounds?

@timhaines
Copy link
Contributor

@frozeman using something similar to the script @avital put together (i.e. easily reproducible) can you show a pattern that continues to increase memory usage long term? You can see from @avital's graph that it grows up to a certain level and then basically stabilizes with spikes and troughs. If you find a recipe that makes memory usage continue to grow, I'd be curious to take a look.

@avital
Copy link
Contributor

avital commented Jun 21, 2013

(I don't think there should be any problem with dynamic loading of template)

@frozeman
Copy link
Author

hi, i will try to get a reproduceable example. Could it may have to do with the Collections?

@frozeman
Copy link
Author

Hi,

so thank you for your time, i now made a sample project showing my issues.
I figured out that its not the template engine, as far as i can tell.

it seems more that it is the local collection.

To explain my situation and the test project.
I want to build an app using meteor, because i think its awesome :)
But we have already an API, so i only want to use the fronted part of Meteor. I do this by copying the "static" part and the "static_cacheable" folders to the main folder and put it on an apache or ngnix.

He then tries to connect to the meteor server, so i added the "go-offline" (meteorite) package, which just cancels the connection and sets the timeout to a really long number. This is a little bit hacky, but i guess you guys will make it easier in the future to remove core packages, for these purposes (am i right :)

so what i did i want to use you the local collection, because it has the nice reactivity. but to use it i need to clear the collection once in a while (e.g. when fetching new data from the API), so i wrote a script, which basically wraps the insert() method and stores the IDs, so i later can remove them one, by one, to clean up the collection.

in my test project i did exactly that. run it a while and you will see it goes over 150MB+ with time and this by only adding 100 items to the Collection and removing them on each page switch.

i think it should be possible to fill and remove the collection locally, without stacking up memory..?

Any ideas whats wrong here?

I uploaded the test package here:
https://dl.dropboxusercontent.com/u/20835796/memoryTestProject.tar.gz

and here is a screenshot from my test run:

screen shot 2013-06-21 at 22 07 16

@frozeman
Copy link
Author

Btw, when using only templates without data, but with some text it uses 21MB at peaks, which is - like you said - already a lot of memory.

screen shot 2013-06-21 at 22 29 13

@timhaines
Copy link
Contributor

@frozeman It'll make it easier for people to view and discuss your project if you put it on github.

@frozeman
Copy link
Author

absolutely right :)
Sadly i have the right repo on my other computer, and the one i send is minified (my mistake)

I put it anyway in a repo and will replace it with the unminified version in the next days:
https://github.com/frozeman/MeteorMemoryTest

Thanks for taking the time, its a serious issue for me and i would like to know what either i do wrong, or whats wrong in the collection..

@frozeman
Copy link
Author

So i add the real version to github, with a small read me.
https://github.com/frozeman/MeteorMemoryTest

Please try it yourself and give me feedback.
Thanks again.

@avital
Copy link
Contributor

avital commented Jun 22, 2013

Please supply a short snippet of JavaScript that we should run in order to
reproduce the problem (as I did for todos)
On Jun 22, 2013 5:14 AM, "Fabian Vogelsteller" notifications@github.com
wrote:

So i add the real version to github, with a small read me.
https://github.com/frozeman/MeteorMemoryTest

Please try it yourself and give me feedback.
Thanks again.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1157#issuecomment-19855847
.

@frozeman
Copy link
Author

You can just start the app. It already contains the script to flip pages as well as add and remove data to the collection.

I wrote also something in the readme.

@frozeman
Copy link
Author

I guess i know where the error lays.

I fill the local db with insert() and then remove all items by id with remove() (i wrote a wrapper for the insert method, which collects all ids of inserted documents). It seems that this way the dc doesn't get cleaned properly..

i need to clean the local db, as i only want to use the frontend part of meteor, due to an already existing API backend.

what is the right method to do that?

@avital
Copy link
Contributor

avital commented Jun 24, 2013

To clear a collection, call collection.remove({}). I'll go ahead and create a small test that only adds and removes from a local collection in a way similar to what you did, to see if that also causes a memory leak.

@avital
Copy link
Contributor

avital commented Jun 24, 2013

Notably, collection.remove({}) only works on the server (because of our security model). If you want to clear a collection from the client, define a server method (make sure to check permissions).

@frozeman
Copy link
Author

The problem is that I'm not using the full stack of meteor, I need to decouple the frontend part, as our backend infrastructure already exists.
I wrote an email to you guys and the response was in favor for that idea.

I believe it would definitely push the widespread usage of meteor, as it can faster used by real world apps, like ours.

It should be possible to decouple the frontend part and only use the local collection. As I,m using an API backend, I have to implement the security myself.

Would be nice if there is an option to remove the local collection security.

Fabian Vogelsteller
Gesendet mit Sparrow (http://www.sparrowmailapp.com/?sig)

Am Montag, 24. Juni 2013 um 19:44 schrieb Avital Oliver:

Notably, collection.remove({}) only works on the server (because of our security model). If you want to clear a collection from the client, define a server method (make sure to check permissions).


Reply to this email directly or view it on GitHub (#1157 (comment)).

@avital
Copy link
Contributor

avital commented Jun 24, 2013

Let's keep this thread about the potential memory leak. If you'd like to discuss collection strategies, please post separately to meteor-talk.

@frozeman
Copy link
Author

Ok i will, but did you tried my example project?
i wrote it and included only what i guess causes the leaking.

even if its not the proper use, the local collection should not stack up memory when adding and removing items..?

@frozeman
Copy link
Author

Could you give me a hint, while the remove() method is not cleaning the Local Collection properly?

I could change to my own objects which i populate, but i want the nice reactivity, when it comes to {{#each}} (where it only updates the changed one)

If local collection isn't the right way, how can i achieve the same on a local collection?

@frozeman
Copy link
Author

I made some digging and it seems that the problem is that the meteor _LivedataConnection stores these even removed elements in a queue, so it can be synced later with the server.

as i want to use meteor without the server part, is there a way to remove the live data package or deactivate the syncing somehow?

@avital
Copy link
Contributor

avital commented Jun 25, 2013

This is not a support thread. If you have questions about how to use Meteor, try meteor-talk, StackOverflow or IRC.

Now, back to the memory leak issue. The first thing I did with your example is to remove external dependencies (router, go-offline). Next time you submit a repro it must not require using 'mrt' to run. I'll make sure to update our documentation to make this explicit.

I also added a button to start switching screens back and forth for exactly one minute, so that we can look at the results more scientifically. Here is my repo: https://github.com/avital/MeteorMemoryTest. When I run it for one minute, this is the memory consumption graph I get. (Notably, after the full run I clicked the 'Collect Garbage' button).
no external deps

Then I tried to isolate the cause of this memory leak, by trying: (1) disabling only the collection modifying code; (2) disabling only the display switching code

When I disable the collection modifying code, I get the following graph.
disable collection stuff
[which still seems has a (minor) memory leak]

When I disable the display switching code (namely, I don't change the Session variable), I get the following graph.
disable changing screens
[which seems to have no memory leak]

So, my interpretation here is that the collection stuff is simply not the cause of the memory leak (though it is if you have the 'go-offline' package added, but that's not officially supported by Meteor). I think we do have some leak in Spark, but I think it's relatively minor. I think the main cause of your original big memory leak is the 'go-offline' package.

I'll talk with some other core devs about this and continue investigating.

dgreensp added a commit that referenced this issue Jun 25, 2013
@avital
Copy link
Contributor

avital commented Jun 25, 2013

Hi @frozeman, @dgreensp and I found the cause of the memory leak in Spark. Apparently we were creating a function that closed over an object that had references to essentially all of the information generated by all previous renders of your templates. So they never got GCd.

Memory consumption graphs (before 49e9813):
no external deps

After:
new

Thanks for helping us find this.

@avital avital closed this as completed Jun 25, 2013
@frozeman
Copy link
Author

Awesome, thanks!

@frozeman
Copy link
Author

Hey, thanks again for looking into this.

I updated your test using a unmanaged local collection and still see a lot of memory stacking up.
See my repo here:
https://github.com/frozeman/MeteorMemoryTest

screen shot 2013-06-26 at 16 25 25

@avital
Copy link
Contributor

avital commented Jun 26, 2013

The bug was fixed on the devel branch, so it's not released yet. You can follow the slow start instructions to run Meteor from a git checkout.

Here is the graph I get by running your repo on devel (after the one minute experiment I initiated a full GC):
screen shot 2013-06-26 at 11 00 12 am

@frozeman
Copy link
Author

i used meteorite adding

 "meteor": {
    "branch": "devel",
    "git": "https://github.com/meteor/meteor.git"
  }

to the smart.json.

i will try your way out tomorrow.

Thanks again

@frozeman
Copy link
Author

frozeman commented Jul 2, 2013

Thanks again for the fix!

just a short note why i experienced even after a memory leak in my app.

I bound an onLoad event to images in a template, and i didn't removed them after the template was destroyed. This stacked up memory.

Sadly meteors own events don't support the onLoad event, so i have to use jQuery for that.
But i don't know how to properly unbind events when the template gets destroyed, as the elements are not anymore in the DOM when the destroyed method is called.

How is the proper way to unbind events on template destruction?
I created a meteor-talk post for that:
https://groups.google.com/forum/#!topic/meteor-talk/8Igzhs0laN4

Thanks again.

@avital
Copy link
Contributor

avital commented Jul 2, 2013

Yes, indeed as you thought, meteor-talk is the right place for this
question.
On Jul 2, 2013 4:04 AM, "Fabian Vogelsteller" notifications@github.com
wrote:

Thanks again for the fix!

just a short note why i experienced even after a memory leak in my app.

I bound an onLoad event to images in a template, and i didn't removed them
after the template was destroyed. This stacked up memory.

Sadly meteors own events don't support the onLoad event, so i have to use
jQuery for that.
But i don't know how to properly unbind events when the template gets
destroyed, as the elements are not anymore in the DOM when the destroyedmethod is called.

How is the proper way to unbind events on template destruction?
I created a meteor-talk post for that:
https://groups.google.com/forum/#!topic/meteor-talk/8Igzhs0laN4

Thanks again.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1157#issuecomment-20339511
.

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

No branches or pull requests

4 participants