Memory leaks

[JeremyTCD]

Hi, first off thanks for the hard work on snap.

I found that my animations were leaking memory (and found that the problem could be reproduced by debugging the demos > snap-mascot page) and did some digging. Turned out that the issue was with the following handlers:

eve.once("mina.finish." + anim.id, function () {
    delete el.anims[anim.id];
    callback && callback.call(el);
});
eve.once("mina.stop." + anim.id, function () {
    delete el.anims[anim.id];
});

When the mina.stop event triggers the handler for the mina.finish event is not deleted and vice versa. The handlers remain in memory until the application is terminated since they are referenced by eve's events property.

I fixed the issue by removing mina.stop altogether and triggering mina.finish in stopit. There is still a less major memory leak issue involving the handlers but I haven't fixed it yet.

Hope this helps someone, cheers.

[janhoogeveen]

My company is also having issues with animations. As a test I tried to a simple infinite loop with add, and remove methods. These methods add a single element, update the value in the next method to the index of the loop, and lastly remove the element altogether. I can easily go to a loop count of 130.000 and still only use 12MB memory.

When I add a very simple animation in the update method using element.animate({y: newValue}), I get a huge memory leak.

[ibrierley]

Possible to put a jsfiddle up ?

[janhoogeveen]

Hi @ibrierley, definitely.

The first pen shows a simple loop where we use transitions and resolve a promise in the transition callback. This one slowly but very steadily climbs in memory (over 100MB after 30.000 loops), as seen when using the task manager in Google Chrome. The timeline shows a similar view where nodes and memory heap are growing very slowly.

http://codepen.io/janhoogeveen/pen/YXPdLy

The second pen doesn't use a transition and directly updates the y value. This one can loop forever and you definitely see that the garbage collector is doing a way better job here. It resets to 10MB live memory pretty often.

http://codepen.io/janhoogeveen/pen/ZGGBQw

[JeremyTCD]

Hey guys, sorry about forgetting to post my full solution. I fixed the memory leak by adding an option for elementId in the mina function. I changed:

    mina = function (a, A, b, B, get, set, easing) {
        var anim = {
            id: ID(),
       ...}

to

    mina = function (a, A, b, B, get, set, easing, elementId) {
        var anim = {
            id: elementId,
        ...}

then in elproto.animate

    ...
    var now = mina.time(),
        anim = mina(fkeys, tkeys, now, now + ms, mina.time, function (val) {
            var attr = {};
            for (var key in keys) if (keys[has](key)) {
                attr[key] = keys[key](val);
            }
            el.attr(attr);
        }, easing,
        /* use element's id as animation callback id*/
        el.id);
    ...

The random Id generator, ID(), causes a new object (mina.n.callback.n.randomId, holds stop and finish callbacks) to be initialized every time an animation is called on a snap element. These objects are never deleted (as they are referenced by eve’s events property).

By supplying an Id that is consistent for each snap element, mina.n.callback.n.elementId objects are only initialized the first time an animation is called on a snap element. Thereafter when the same element is animated, the mina.n.callback.n.elementId object is reused.

This was a quick and dirty solution, hopefully it helps someone with deeper knowledge of Snap to fix the problem in a clean way though. Cheers.

[DmitryBaranovskiy]

I believe I’ve fixed this issue on dev branch. Could somebody test it out?

[mike-tempest]

@DmitryBaranovskiy I still have the same problem on the dev branch, tested on this demo demos.michaeltempest.com/hill-valley.html

[janhoogeveen]

Dmitry, didn't have the time to test this out yet properly. Sorry for that. Will integrate this within my project and tell you if it helped.

[JeremyTCD]

@DmitryBaranovskiy

It appears to still be leaking for me, heap grows from mina.n.callback.n.randomId objects accumulating over time.

The eve.off additions should really be taking care of the issue though. Will try to help pinpoint the issue when I have some time.

[janhoogeveen]

Quick update, also still seems to be leaking on my part. Checked out the development branch and performed a new build of Snap on my own machine, so definitely using the latest code. Manually checked and the patch was in there.

[mike-tempest]

@DmitryBaranovskiy any progress on this, I've had to strip it from a one page application because of these memory leaks

[vinceprofeta]

@DmitryBaranovskiy What is the status of this?

[elinfante]

Any news on this? I am having big memory leaks and I have narrow it down to this problem. Would be fantastic If someone could update us on this.

[iandanforth]

I'm also seeing this issue. Let me know if there's any way I can assist.

[iandanforth]

@DmitryBaranovskiy I just finished testing a combination of your patch and @JeremyTCD . With both applied my application now has stable memory usage though it takes 5+ minutes to reach that point of stability.

[elinfante]

Hi guys, I have been having this issue for a while so very keen on having it sorted. @iandanforth could assist on what to do? Thank you very much!

[JeremyTCD]

Hi guys, the reason why heap size still increases over time is because of code optimizations by optimizing compilers (crankshaft in v8, https://wingolog.org/archives/2011/08/02/a-closer-look-at-crankshaft-v8s-optimizing-compiler). Optimizing compilers optimize functions when they have been called a certain number of times. In the process they collect inline cache (http://mrale.ph/blog/2012/06/03/explaining-js-vms-in-js-inline-caches.html) data and generate new code that causes heap size to grow. Mitigating these issues requires fundamental changes to the way Snap is designed.

[ThomasBrierley]

Hi @JeremyTCD I'm not sure VM optimising compilers are part of this problem, compiled code aught to be pruned as part of GC. Either way, expired event objects are still accumulating in the JavaScript heap, and it's Snap's responsibility to free references to these for the GC.

I found another accumulation of references within the event library "Eve". I think these might be the last of them: The event constructor / destructor are asymmetrical, the constructor walks the event tree creating any absent i-nodes in the event path before creating it's target event; the destructor removes it's target event from the tree but does not remove empty nodes in the event path above. For most events this results in a trail of redundant parent nodes.

Fixing the destructor to remove empty parents was relatively easy; ensuring the process is reasonably performant was a bit more tricky. Periodic purges cause larger GC spikes, so it needs to be integral to the destructor, which means event path search and removal should be optimised - which requires a bit of careful re-factoring. In my tests the GC reliably reclaims the heap (dispatching thousands of Snap animations per second), with reasonable performance (without pooling GC gota happen).

I actually did this three months ago, but Snap and Eve have become stagnant so i had little motivation to tidy it into a PR. I'll do a PR for Eve if there is still interest ... i guess it's worth it if people have the option to merge it into their own fork :)

[ibrierley]

It would be interesting to see a new fork that others worked on (and that others could work on for future, its annoying to see folk do work and it go nowhere really, and its prevent me). As it is, Snap is stagnant as you say, and its holding it back which is a shame.

I do think there is enough interest, so I would release the PR if you are still able, it feels fairly significant.

Good to see another Brierley on Snap ;).

[ThomasBrierley]

Ok - This is just a simple fix, re-factoring for better overall performance got messy. The Eve bind methods are already quite heavy so this doesn't add much, most of the work is still done in the GC.

For convenience I've built this on my fork along with two other small fixes I've done, but please don't rely on it being here permanently: https://github.com/ThomasBrierley/Snap.svg/releases/tag/memory-leak-fix

Here's a basic test running 1000 animations per second. It would be great if anyone could let us know this works for your test case too: http://jsfiddle.net/afv3gdvp/

Oh yeah and Snap on the name @ibrierley :P (sorry)

[skyduck0205]

I have a demo project which uses snap.svg with multiple svg elements and manipulating some complex svg path animation in an infinite loop and encountered memory leak problem too. Thanks to @ThomasBrierley, after use the fixed one the situation is now strongly improved!

Here's the heap snapshots represent the state at the beginning and after 200, 1000 times the animation was executed. The left one is the original Snap.svg 0.4.1 and right one is the memory-leak-fix version.

[ThomasBrierley]

Thanks @skyduck0205 that looks promising.

[PPaques]

@ThomasBrierley Just Tested on my company SVG Dashboard.

50Mb to 17Mb after one hour.

Great Work !

[ThomasBrierley]

Thanks @Pirou01 :)

Just to clarify for anyone else testing this: there should be some heap usage, the exact amount will depend greatly on what you are doing in the page. The leak behaviour is when it grows unbound, e.g 2MiB then 4MiB then 8MiB then 16Mib then 32MiB... and so on.

The behaviour we want is a stable usage where it eventually settles once the rate of animation calls and GC have met e.g: 2Mib then 4Mib then 8Mib then 10Mib then 10Mib then 10Mib... and so on. it doesn't matter if that stable number is higher, so long as it doesn't continue to grow while making the same rate of calls.

As can be seen in @skyduck0205 's 2nd screenshot, the 0.1MiB difference is most likely either other things outside of Snap in the page or a slight change in the rate of animation calls.

[ThomasBrierley]

Summary for people catching up with this issue, TODO:

• [x] Merge fix into Eve adobe-webplatform/eve#27
• [x] Tag and npm publish Eve
• [x] Bump Eve version in Snap's package.json

[ThomasBrierley]

Thanks! :smile: :beer:

[DmitryBaranovskiy]

@ThomasBrierley No, Thank you.