scratchfoundation / scratch-gui

Graphical User Interface for creating and running Scratch 3.0 projects.
https://scratchfoundation.github.io/scratch-gui/develop/
BSD 3-Clause "New" or "Revised" License
4.44k stars 3.54k forks source link

Memory Leak when reload a new project or click File->New #6309

Open altjz opened 3 years ago

altjz commented 3 years ago

Expected Behavior

Should release the memory(Including GPU Memory) when click File->New Or reload a new project

Actual Behavior

  1. before load any project before
  2. load a large project after
  3. after click File->New clicknew

You can see the Memory usage only reduce 5%, GPU Memory usage not at all.

This will be a disaster on Scratch Desktop, you reload a project or create new A project, it won't release any memory until you restart the desktop.

Steps to Reproduce

  1. Open a large project like this FIFA 19
  2. Open the Task Manager (chrome)
  3. Click File->New

Operating System and Browser

Mac OS 10.15.6 Chrome 86 Same on Windows

Same issue on Scratch Desktop

BryceLTaylor commented 3 years ago

Can a user perceive this issue? What do they have to do for this to be noticeable to the user?

cwillisf commented 3 years ago

There could be many reasons for this, so it might make sense to split this into multiple issues as we discover causes. We probably do have one or more resource leak(s), but in addition to that I suspect the browser is not very aggressive in decreasing the space allocated to JavaScript.

One thing to look into: at first glance it looks like we might not reliably unregister costumes from the renderer when a sprite or costume gets deleted. If so, we should call runtime.renderer.destroySkin(...) to clean up.

cwillisf commented 3 years ago

My guess is that most users won't see any effect from this, but some might see a slowdown of Scratch or their overall computer (swap thrashing). In the worst case I think it would lead to the browser tab or app crashing (or being terminated due to exceeding limits) which could lead to data loss, but I suspect that's very rare.

If we get more information that suggests this is worse then we should adjust the labels, but for now I think "low impact" and "medium severity" is a reasonable estimate.

altjz commented 3 years ago

There could be many reasons for this, so it might make sense to split this into multiple issues as we discover causes. We probably do have one or more resource leak(s), but in addition to that I suspect the browser is not very aggressive in decreasing the space allocated to JavaScript.

One thing to look into: at first glance it looks like we might not reliably unregister costumes from the renderer when a sprite or costume gets deleted. If so, we should call runtime.renderer.destroySkin(...) to clean up.

I found this probably cause by the renderer._allSkins.

the below code would reduce 90% memory usage from last project. But I can't tell no other side effect behind this behavior.

Reproduce Step (suggest use incognito mode)

  1. Open the official site: https://scratch.mit.edu/projects/editor/
  2. Take Snapshot on devtool (Memory Tab), probably around 30 - 40 MB.
  3. Open a Large Project, (use File->Load from your computer) eg. FIFA19
  4. Take Snapshot on devtool, probably around 850MB (use the FIFA 19 project)
  5. Click File -> New
  6. Take Snapshot, only reduce 25 MB
  7. Run the below code in console
    
    // Find React Instance
    function getReactInstance(dom) {
    for (const k in dom) {
        if (k.startsWith('__reactInternalInstance$')) {
            return dom[k];
        }
    }
    }
    // Find VM
    function findVm(reactInstance) {
    if (reactInstance.return) {
        if (reactInstance.return.stateNode) {
            if (reactInstance.return.stateNode.props && reactInstance.return.stateNode.props.vm) {
                return reactInstance.return.stateNode.props.vm;
            }
        }
        return findVm(reactInstance.return);
    }
    }
    let vm = findVm(getReactInstance(document.querySelector(".injectionDiv").parentElement));

// execute the dispose() for all Skins vm.runtime.renderer._allSkins.forEach((skin) => { if (skin) { skin.dispose(); } }); // clear the skin array vm.runtime.renderer._allSkins = []


8. Click `Collect garbage` icon then take a snapshot
9. Reduce 90% memory usage

![after](https://user-images.githubusercontent.com/10369139/97675097-a4c42a00-1ac9-11eb-9477-ddeb9993fea1.png)
apple502j commented 3 years ago

@cwillisf Note that the changes above will be blocked by LLK/scratch-vm#2661 because that bug currently causes clones to keep using the deleted costume.