haltu / muuri

Infinite responsive, sortable, filterable and draggable layouts
https://muuri.dev
MIT License
10.77k stars 644 forks source link

Idle Muuri Board crashing browser? #480

Closed ibrychgo closed 3 years ago

ibrychgo commented 3 years ago

While everything is going well with our board...I've discovered that a board left idle for 20-30 minutes crashes the browser ('Aw Snap!') without errors logged.

When I monitor memory usage, it looks like pe._onWorkerMessage is growing 2 mb every second. Every now and then it goes down a bit but mostly it's a straight path up.

I removed all of our watchers and database connections, so ONLY the muuri board initialization code runs. And, in an idle state, none of our functions are firing. But that webworker keeps growing until crash.

My HTML and javascript for instantiating the boards is nearly identical to the kanban demos. And, since this is an idle test, none of the functions that fire after drag/drop are being called.

Since this is in the webworker...I'm unsure how to proceed investigating. Any ideas?

ibrychgo commented 3 years ago

So after more testing, disabling different parts of the muuri declaration, I narrowed it down to Muuri's own even listeners.

It seems that layoutStart, if enabled, immediately causes this 1.5 - 2mb memory increase every second. Even if all other event listeners are disabled.

If layoutStart is not declared, memory seems to grow and decrease as expected. Growing during activity and cleaning up after.

We were only using it because we thought it was necessary as it was included in some of Muuri's kanban examples so, for our project, this is resolved.

The offending listener:

 .on('layoutStart', function () {
          // Let's keep the board grid up to date with the
          // dimensions changes of column grids.
           boardGrid.refreshItems().layout();
     })

But if anyone DOES need to listen for layoutStart...I have a feeling they will eventually run out of memory and crash.

niklasramo commented 3 years ago

@ibrychgo Interesting! Am I getting this right that if I add an empty layoutStart listener memory allocation will keep growing or is the problem in the boardGrid.refreshItems().layout(); part inside that listener? Also, if possible, could you provide a reduced test case (in e.g. codepen/codesandbox) which demonstrates this problem? Will be easier for me to debug it if I can jump straight to the meat of it.

niklasramo commented 3 years ago

As a temporary fix you can force Muuri to run all layout calculations synchronously in main thread too. Just do this once somewhere, before instantiating any grids:

Muuri.defaultPacker.destroy();
Muuri.defaultPacker = new Muuri.Packer(0);

This code will destroy the default Packer instance and replace it with a new Packer instance that is explicitly told to spawn 0 workers.

ibrychgo commented 3 years ago

Correct.

An empty layoutStart listener causes memory use to grow.

If it's completely empty, it grows but at a modest amount (around 24k/second.) But that amount does keep increasing (i.e. after 15 seconds it's at 123k/second.) I've not tested this state long enough to see it crash.

If the listener includes boardGrid.refreshItems().layout(); memory increases about 1.5-2mb per second until crash.

I've never made a codepen before but I'm willing to try later today. :) I'll post the link here to let you know.

Thanks for being so engaged and responsive!

thednp commented 3 years ago

@niklasramo can this fix help with #473 ?

niklasramo commented 3 years ago

@thednp I don't think this issue is related to #473.

@ibrychgo It turns out that there is a memory leak in the Packer code and just created a fix for it here: #486. Would you mind testing if that PR fixes the memory leak you were having?

ibrychgo commented 3 years ago

I would if I were more skilled in git. But unfortunately, my muuri integration is via the CDN.

Switching to the asynchronous packer as you had suggested not only fixed the issue for me, but all of the flickering that was happening when boards were updated went away as well and I was able to ditch a bunch of unnecessary (and buggy) logic I was writing to keep it looking smooth.

Bryan Lewis Managing Partner

openmotive 3110 North Greenview Suite 600 Chicago, IL 60657

p+ 773.230.7122 | f+ 866.518.9854 @. @.>

On Wed, Jul 7, 2021 at 3:30 PM Niklas Rämö @.***> wrote:

@thednp https://github.com/thednp I don't think this issue is related to #473 https://github.com/haltu/muuri/issues/473.

@ibrychgo https://github.com/ibrychgo It turns out that there is a memory leak in the Packer code and just created a fix for it here: #486 https://github.com/haltu/muuri/pull/486. Would you mind testing if that PR fixes the memory leak you were having?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/haltu/muuri/issues/480#issuecomment-875873883, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNZNEMETXLIZP7GIABCDSDTWSTONANCNFSM46PHE2AQ .

niklasramo commented 3 years ago

@ibrychgo No need to use git for testing this one, here is a direct link to the muuri lib in that PR: https://cdn.jsdelivr.net/gh/haltu/muuri@r0.9.5/dist/muuri.js. If you still can recreate the memory leak in your own code you can then just see if swapping the CDN link fixes it.

niklasramo commented 3 years ago

This should be now fixed with the latest release: https://github.com/haltu/muuri/releases/tag/0.9.5