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 leak in CanJS template/model #1393

Closed
jondubois opened this Issue Jan 6, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@jondubois
Contributor

jondubois commented Jan 6, 2015

When passing observables obtained from can.Model.findAll() or can.Model.find() into a can.view.mustache template generator, it appears to leak memory. The leak goes away when you call myobservable.attr() instead of passing the myobservable object directly to the template generator function. It doesn't matter whether the data comes from a real AJAX call or is provided by a fixture.

Here is a simple test to reproduce the issue (one button causes memory to leak, and the other doesn't - It just rerenders a template many times):

Note that you need to replace the all dependencies with their appropriate paths on your system.
To check memory consumption in Chrome, use the profiles tab of the developer console and take a memory snapshot before and after each test.

<!DOCTYPE html>
<html>
<head lang="en">
    <meta charset="UTF-8">
    <title>CanJS View Memory Leak Test</title>

    <script type="text/javascript" src='../../steal/steal.js'></script>
    <script type="text/javascript">
        steal(
            'jquery',
            'can/util',
            'can/view/mustache',
            'can/model',
            'can/util/fixture',

            function($, can) {

                can.fixture("/api/student/", function() {
                    return [
                        {
                            id: 0,
                            name: 'John Smith',
                            age: 15,
                            classGroup: '7A'
                        },
                        {
                            id: 1,
                            name: 'Alice Brown',
                            age: 14,
                            classGroup: '6B'
                        },
                        {
                            id: 2,
                            name: 'Bob Green',
                            age: 15,
                            classGroup: '7A'
                        },
                        {
                            id: 3,
                            name: 'Tim Tam',
                            age: 16,
                            classGroup: '7B'
                        }
                    ];
                });

                can.Model.extend('Models.Student',
                    /** @static */
                    {
                        create: 'NotImplemented_RESTAPI',
                        destroy: 'NotImplemented_RESTAPI',
                        findOne: 'NotImplemented_RESTAPI',
                        findAll: '/api/student/',
                        update: 'NotImplemented_RESTAPI',
                        attributes: {
                            id: 'number',
                            name: 'string',
                            age: 'number',
                            classGroup: 'string'
                        }

                    },
                    /** @prototype */
                    {});

                var mainTemplate = can.view.mustache(
                    '<input class="start-test-with-leak-button" type="button" value="Start test (with leak)" /> <input class="start-test-without-leak-button" type="button" value="Start test (without leak)" /><div class="main-area"></div>'
                );

                $(document.body).append(mainTemplate());

                var mainArea = $('.main-area');
                var startTestWithLeakButton = $('.start-test-with-leak-button');
                var startTestWithoutLeakButton = $('.start-test-without-leak-button');

                var withLeak = true;

                var addViewToDOM = function() {
                    mainArea.empty();

                    var studentsTemplate = can.view.mustache(
                        '<div>{{#each students}}Syllabus ID: {{id}}, name: {{name}}, age: {{age}}, class group: {{classGroup}}<br />{{/each}}</div>'
                    );

                    var studentsDef = Models.Student.findAll();

                    studentsDef.done(function(students) {
                        var output;

                        if (withLeak) {
                            output = studentsTemplate({
                                students: students
                            });
                        } else {
                            output = studentsTemplate({
                                students: students.attr()
                            });
                        }

                        mainArea.append(output);
                    });
                };

                var startTest = function() {
                    console.log('STARTED TEST');

                    addViewToDOM();
                    var addViewInterval = setInterval(addViewToDOM, 500);
                    setTimeout(function() {
                        clearInterval(addViewInterval);
                        console.log('FINISHED TEST');
                    }, 10000);
                };

                startTestWithLeakButton.on('click', function() {
                    withLeak = true;
                    startTest();
                });

                startTestWithoutLeakButton.on('click', function() {
                    withLeak = false;
                    startTest();
                });
            }
        );
    </script>
</head>
<body>

</body>
</html>
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 8, 2015

Contributor

Thanks for posting this.

Here is a simple test to reproduce the issue

Can you simplify it a bit more? It's probably possible to create this situation without can.Model or can.fixture.

I'm asking because I can't follow the code immediately. I would have to setup this test, which would take some time, and really dive in.

If I can follow the code, it's possible I can immediately spot the problem and advise on a fix.

Thanks!

Contributor

justinbmeyer commented Jan 8, 2015

Thanks for posting this.

Here is a simple test to reproduce the issue

Can you simplify it a bit more? It's probably possible to create this situation without can.Model or can.fixture.

I'm asking because I can't follow the code immediately. I would have to setup this test, which would take some time, and really dive in.

If I can follow the code, it's possible I can immediately spot the problem and advise on a fix.

Thanks!

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 8, 2015

Contributor

Does this happen stache mustache?

(edit stache for mustache)

Contributor

justinbmeyer commented Jan 8, 2015

Does this happen stache mustache?

(edit stache for mustache)

@Tarabyte

This comment has been minimized.

Show comment
Hide comment
@Tarabyte

Tarabyte Jan 9, 2015

I've set up a fiddle. Not sure it is confirming the issue. http://jsfiddle.net/tarabyte/8g0qv4xj/
Well, the memory consumption is growing both cases. But not as if it was leaking something having model instances in closure.

Tarabyte commented Jan 9, 2015

I've set up a fiddle. Not sure it is confirming the issue. http://jsfiddle.net/tarabyte/8g0qv4xj/
Well, the memory consumption is growing both cases. But not as if it was leaking something having model instances in closure.

@jondubois

This comment has been minimized.

Show comment
Hide comment
@jondubois

jondubois Jan 9, 2015

Contributor

@justinbmeyer Note sure, I haven't tried with stache yet.

@Tarabyte On my machine, the script I provided only leaks when you click on the 'Start test (with leak)' button.
In the slightly modified version you provided in JSFiddle, both leak. The first one leaks almost 3MB and the second one only leaks slightly over 1MB - So one is leaking more than the other... Could this inconsistency be related to fiddler itself?

When checking for memory leaks, I tend to check in complete isolation in a fresh tab - I'm not sure how jsfiddle interacts with all this code - It might skew results... I also noticed that jsfiddle runs the code in an iframe which can affect things too - For example, you will find that deleting the iframe cleans up all memory leaks (because iframe memory is sandboxed).

Contributor

jondubois commented Jan 9, 2015

@justinbmeyer Note sure, I haven't tried with stache yet.

@Tarabyte On my machine, the script I provided only leaks when you click on the 'Start test (with leak)' button.
In the slightly modified version you provided in JSFiddle, both leak. The first one leaks almost 3MB and the second one only leaks slightly over 1MB - So one is leaking more than the other... Could this inconsistency be related to fiddler itself?

When checking for memory leaks, I tend to check in complete isolation in a fresh tab - I'm not sure how jsfiddle interacts with all this code - It might skew results... I also noticed that jsfiddle runs the code in an iframe which can affect things too - For example, you will find that deleting the iframe cleans up all memory leaks (because iframe memory is sandboxed).

@jondubois

This comment has been minimized.

Show comment
Hide comment
@jondubois

jondubois Jan 13, 2015

Contributor

@justinbmeyer I will try to reproduce it using a simple Observe instead of the Model + Fixture combination - It might still leak.

Ultimately, something appears to be holding on to references of the data. When I was debugging a while ago, I think I saw something about a _cache property somewhere in 'can' and I remember thinking that this could be the cause... Does that ring a bell?

Contributor

jondubois commented Jan 13, 2015

@justinbmeyer I will try to reproduce it using a simple Observe instead of the Model + Fixture combination - It might still leak.

Ultimately, something appears to be holding on to references of the data. When I was debugging a while ago, I think I saw something about a _cache property somewhere in 'can' and I remember thinking that this could be the cause... Does that ring a bell?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 13, 2015

Contributor

Yes, the model layer keeps instances, but only while they are bound. Clearing the page should clean them up.

Sent from my iPhone

On Jan 13, 2015, at 3:54 PM, Jonathan Gros-Dubois notifications@github.com wrote:

@justinbmeyer I will try to reproduce it using a simple Observe instead of the Model + Fixture combination - It might still leak.

Ultimately, something appears to be holding on to references of the data. When I was debugging a while ago, I think I saw something about a _cache property somewhere in 'can' and I remember thinking that this could be the cause... Does that ring a bell?


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jan 13, 2015

Yes, the model layer keeps instances, but only while they are bound. Clearing the page should clean them up.

Sent from my iPhone

On Jan 13, 2015, at 3:54 PM, Jonathan Gros-Dubois notifications@github.com wrote:

@justinbmeyer I will try to reproduce it using a simple Observe instead of the Model + Fixture combination - It might still leak.

Ultimately, something appears to be holding on to references of the data. When I was debugging a while ago, I think I saw something about a _cache property somewhere in 'can' and I remember thinking that this could be the cause... Does that ring a bell?


Reply to this email directly or view it on GitHub.

@justinbmeyer justinbmeyer self-assigned this Feb 11, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 11, 2015

Contributor

@jondubois I'm going to look into this Friday. Anyway we can chat about it sometime on Friday?

Contributor

justinbmeyer commented Feb 11, 2015

@jondubois I'm going to look into this Friday. Anyway we can chat about it sometime on Friday?

@justinbmeyer justinbmeyer added this to the 2.2.0 milestone Feb 11, 2015

@justinbmeyer justinbmeyer added the bug label Feb 11, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 13, 2015

Contributor

@jondubois I can confirm there is a leak in Mustache, but not in stache.

Contributor

justinbmeyer commented Feb 13, 2015

@jondubois I can confirm there is a leak in Mustache, but not in stache.

@jondubois

This comment has been minimized.

Show comment
Hide comment
@jondubois

jondubois Feb 15, 2015

Contributor

@justinbmeyer Thanks for looking into this. We will try to migrate our templates to stache.

Contributor

jondubois commented Feb 15, 2015

@justinbmeyer Thanks for looking into this. We will try to migrate our templates to stache.

justinbmeyer added a commit that referenced this issue Mar 3, 2015

justinbmeyer added a commit that referenced this issue Mar 4, 2015

justinbmeyer added a commit that referenced this issue Mar 4, 2015

@daffl daffl closed this in #1487 Mar 4, 2015

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