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 leak with AudioContext #410

Closed
TwitchBronBron opened this issue Nov 15, 2018 · 12 comments

Comments

Projects
None yet
2 participants
@TwitchBronBron
Copy link

commented Nov 15, 2018

I think I found a memory leak when using AudioContext. Here's a some sample code and steps to reproduce:

  1. Click the play button once to warm up the page and wait for it to finish.
  2. Then, in chrome dev tools, take a heap snapshot.
  3. Then click the "play" button again and wait for it to finish.
  4. Then take another heap snapshot. Note the memory usage increases.
  5. Change the html code from var ctx = new sac.AudioContext() to var ctx = new AudioContext() (i.e. use the native AudioContext) and repeat the steps above, Note the memory usage stays the same.

After every play iteration, the memory usage goes up. If you run this same code using the native AudioContext object, the memory usage is stable.

<html>
<body>
    <button id="play" onclick="play()" disabled>Play</button>
    <script type='module'>
        import sac from 'https://dev.jspm.io/npm:standardized-audio-context';
        var ctx = new sac.AudioContext()
        var arrayBuffer;
        async function getArrayBuffer() {
            if (!arrayBuffer) {
                var response = await fetch('https://s3-us-west-2.amazonaws.com/s.cdpn.io/123941/Yodel_Sound_Effect.mp3');
                arrayBuffer = await response.arrayBuffer();
            }
            return arrayBuffer.slice(0);
        }
        window.play = async function () {
            for (var i = 0; i < 100; i++) {
                console.log(`Playing ${i}`);
                let source = ctx.createBufferSource();
                source.buffer = await ctx.decodeAudioData(await getArrayBuffer());
                source.connect(ctx.destination)
                source.start();
                await new Promise((resolve) => {
                    setTimeout(resolve, 50)
                });
                source.stop();
            }
        }
        window.onload = () => {
            document.getElementById('play').removeAttribute('disabled');
        };
    </script>
</body>
</html>

standardized-audio-context memory usage (note how the memory grows over time):
image

Native AudioContext memory usage (note how the memory does not increase over time):
image

@chrisguttandin

This comment has been minimized.

Copy link
Owner

commented Nov 15, 2018

Hi @TwitchBronBron,

many thanks for reporting this. I will try to take a closer look at it later today.

Do you maybe know how we could turn this into an automated test? It would be nice to add a test like this to make sure the memory leak never comes back once it ist fixed.

Some time ago I tried to build a automated memory leak test with puppeteer but it didn't work out. Do you maybe know a way of doing it?

@TwitchBronBron

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

I'm not sure exactly how you run your tests, but if they run in some form of the chrome browser (like headless chrome), then chrome offers the window.performance.memory object that tells you memory statistics. I adapted my test page to show how to calculate the amount of memory INCREASE over a set of tests.

Here are the results when I run this in chrome:

      native audio context:   4,781
standardized-audio-context: 494,848

Garbage collection is unpredictable, so you can't just target 0 memory difference. Sometimes when I run this test, the native test shows negative memory usage over time. That's all due to the fact that we can't control when the garbage collector runs.

However, notice the drastic difference in memory increase over time for standardized-audio-context. That could be how you detect this issue in a test. Anything over, say, 100,000 fails the test. You want to prevent false positives, but you don't want to miss actual memory leaks. I would make your memory leak test use some huge files so the fluxuation is drastic. Like maybe a 1-2 meg file played 10-100 times. That way you can be confident the test will be over a certain threshold.

<html>
<body>
    <button id="play" onclick="test()" disabled>Test</button>
    <script type='module'>
        import sac from 'https://dev.jspm.io/npm:standardized-audio-context';
        var ctx;
        var memoryData = [0, 0, 0, 0, 0, 0, 0];
        var arrayBuffer;
        async function getArrayBuffer() {
            if (!arrayBuffer) {
                var response = await fetch('https://s3-us-west-2.amazonaws.com/s.cdpn.io/123941/Yodel_Sound_Effect.mp3');
                arrayBuffer = await response.arrayBuffer();
            }
            return arrayBuffer.slice(0);
        }

        window.test = async function () {
            ctx = new AudioContext();
            await window.play();
            ctx = new sac.AudioContext()
            await window.play();
        }
        window.play = async function () {
            console.log('Starting tests');
            for (var k = 0; k < memoryData.length; k++) {
                for (var i = 0; i < 100; i++) {
                    //console.log(`Playing ${i}`);
                    let source = ctx.createBufferSource();
                    source.buffer = await ctx.decodeAudioData(await getArrayBuffer());
                    source.connect(ctx.destination)
                    source.start();
                    await new Promise((resolve) => {
                        setTimeout(resolve, 10)
                    });
                    source.stop();
                }
                await new Promise((resolve) => {
                    setTimeout(resolve, 10)
                });
                memoryData[k] = window.performance.memory.usedJSHeapSize;
                console.log('Finished iteration ' + (k + 1));
            }

            var sum = 0;
            for (var i = 1; i < memoryData.length; i++) {
                var thisHeapUsage = memoryData[i];
                var lastHeapUsage = memoryData[i - 1];
                var amountIncrease = thisHeapUsage - lastHeapUsage;
                sum += amountIncrease;
            }
            var average = sum / (memoryData.length - 1);
            console.log('Average heap increase', average);
            console.log(memoryData);
        }
        window.onload = () => {
            document.getElementById('play').removeAttribute('disabled');
        };
    </script>
</body>

</html>
@chrisguttandin

This comment has been minimized.

Copy link
Owner

commented Nov 16, 2018

Hi @TwitchBronBron,

thanks for your detailed response.

The tests run via karma in each supported browser. Currently those browsers are Chrome, Edge, Firefox, Opera & Safari. But for something like a memory leak test I think it is absolutely fine to run it in only one browser.

I made some experiments with window.performance.memory in the past and tried it now again. I initially thought it returns completely unreliable results on my machine. However I now realized that it needs to run on a secure context in order to return correct results.

This brings me to another question. How do you run your test in Chrome? I have to disable web-security in order to circumvent the CORS error. I start Chrome like this: open -a Google\ Chrome --args --disable-web-security --user-data-dir. But when I do so window.performance.memory does not return meaningful numbers. Which is why I can't really run the updated version of your test case.

However I can profile it manually and I can also see that it consumes in increasing amount of memory.

After staring at the code for quite some time I realized that the source never gets disconnected again. When adding source.disconnect(ctx.destination); the memory impact is reduced but still there. I still have to investigate why that's the case.

@TwitchBronBron

This comment has been minimized.

Copy link
Author

commented Nov 19, 2018

I'm not sure why you had to disable web security, or why window.performance.memory doesn't return meaningful results.

I'm not running anything special. I just launch my normal chrome (Version 70.0.3538.102 (Official Build) (64-bit)).

Here's another version of my test code, but this time using an empty buffer rather than loading from a file. It still reproduces the memory leak.

<html>

<body>
    <button id="play" onclick="test()" disabled>Test</button>
    <script type='module'>
        import sac from 'https://dev.jspm.io/npm:standardized-audio-context';
        var ctx;
        var memoryData = [0, 0, 0, 0, 0, 0, 0];

        window.test = async function () {
            ctx = new AudioContext();
            await window.play();
            ctx = new sac.AudioContext()
            await window.play();
        }
        window.play = async function () {
            console.log('Starting tests');
            for (var k = 0; k < memoryData.length; k++) {
                for (var i = 0; i < 100; i++) {
                    let source = ctx.createBufferSource();
                    source.buffer = ctx.createBuffer(1, 1, 22050);
                    source.connect(ctx.destination)
                    source.start();
                    await new Promise((resolve) => {
                        setTimeout(resolve, 10)
                    });
                    source.stop();
                }
                await new Promise((resolve) => {
                    setTimeout(resolve, 10)
                });
                memoryData[k] = window.performance.memory.usedJSHeapSize;
                console.log('Finished iteration ' + (k + 1));
            }

            var sum = 0;
            for (var i = 1; i < memoryData.length; i++) {
                var thisHeapUsage = memoryData[i];
                var lastHeapUsage = memoryData[i - 1];
                var amountIncrease = thisHeapUsage - lastHeapUsage;
                sum += amountIncrease;
            }
            var average = sum / (memoryData.length - 1);
            console.log('Average heap increase', average);
            console.log(memoryData);
        }
        window.onload = () => {
            document.getElementById('play').removeAttribute('disabled');
        };
    </script>
</body>

</html>
@chrisguttandin

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

Hi, @TwitchBronBron,

did you maybe serve the site on localhost? I opened the file directly in Chrome initially. Now that I use a simple server to serve it on localhost, I can finally use window.performance.memory.

I think I found the reason why this is happening. Currently every AudioNode needs to know it's inputs and outputs. This does prevent the garbage collection from collecting AudioNodes which are connected. I do have an idea on how to overcome this design flaw but unfortunately it is not an easy fix. I will need some time to first test and then implement it.

In the meantime you can make sure that you disconnect every node that you connected before. This will remove the reference which prevents garbage collection.

source.stop();
source.disconnect();

When you disconnect the source after stopping it the tests do still sometimes show a memory increase. However this seems to be a symptom of memory pressure and not a memory leak. When you run the tests a couple of times the overall memory usage doesn't rise.

@TwitchBronBron

This comment has been minimized.

Copy link
Author

commented Dec 17, 2018

Ah, yes, I was serving the page over localhost. I'm sorry for not mentioning that!

Calling disconnect on the source does seem to help, but not completely mitigate the issue. After every test run, I took a heap snapshot. It's still growing over time.

image

Here's my updated test code.

<html>

<body>
    <button id="play" onclick="test()" disabled>Test</button>
    <script type='module'>
        import sac from 'https://dev.jspm.io/npm:standardized-audio-context';
        var ctx;
        var memoryData = [0, 0, 0, 0, 0, 0, 0];

        window.test = async function () {
            ctx = new AudioContext();
            await window.play();
            ctx = new sac.AudioContext()
            await window.play();
        }
        window.play = async function () {
            console.log('Starting tests');
            for (var k = 0; k < memoryData.length; k++) {
                for (var i = 0; i < 100; i++) {
                    let source = ctx.createBufferSource();
                    source.buffer = ctx.createBuffer(1, 1, 22050);
                    source.connect(ctx.destination)
                    source.start();
                    await new Promise((resolve) => {
                        setTimeout(resolve, 10)
                    });
                    source.stop();
                    //disconnect the source
                    await new Promise((resolve) => {
                        setTimeout(resolve, 10);
                   });
                   source.disconnect();
                }
                await new Promise((resolve) => {
                    setTimeout(resolve, 10)
                });
                memoryData[k] = window.performance.memory.usedJSHeapSize;
                console.log('Finished iteration ' + (k + 1));
            }

            var sum = 0;
            for (var i = 1; i < memoryData.length; i++) {
                var thisHeapUsage = memoryData[i];
                var lastHeapUsage = memoryData[i - 1];
                var amountIncrease = thisHeapUsage - lastHeapUsage;
                sum += amountIncrease;
            }
            var average = sum / (memoryData.length - 1);
            console.log('Average heap increase', average);
            console.log(memoryData);
        }
        window.onload = () => {
            document.getElementById('play').removeAttribute('disabled');
        };
    </script>
</body>

</html>
@chrisguttandin

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2019

Hi @TwitchBronBron, I think I discovered the root problem.

At its core standardized-audio-context uses several WeakMaps to keep track of the relationships between the nodes in the audio graph. WeakMaps do not seem to be as weak as I thought they are. Their memory footprint seems to keep growing over time.

When I open about:blank in Chrome and Firefox and inspect the memory, I get an initial value of 1.9 MB in Chrome and 0.22 MB in Firefox. This doesn't change much when I create a global WeakMap by pasting the following line in the console:

w = new WeakMap();

If I then go ahead and cause a lot of trouble by adding a lot of objects to the WeakMap the memory usage increases rapidly.

intervalId = setInterval(() => { for (let i = 0; i < 1000; i += 1) { w.set(new Map(), new Set()); } }, 1);

This is somehow expected because I create a lot of objects. However they can all be collected and therefore I would expect the size of the used memory to go back to the initial value once I cancel the interval.

clearInterval(intervalId);

But when I do this and trigger the garbage collection several times, I still see up to 18 MB memory usage in Chrome and up to 18 MB memory usage in Firefox as well. I repeated the test several times and these are the highest numbers.

My guess is that both browsers increase the internal registry of the WeakMap and never shrink it again when it is not needed anymore.

If I remove the reference to the WeakMap itself by running w = null the memory usage gets back to the initial value. Firefox does also clean up the memory itself after a while.

I'm not sure if the browser vendors consider this to be a bug. It could very well be a performance optimization instead.

I try to come up with a testing mechanism that does not rely on the size of the WeakMaps as this is outside of the control of this library. Please let me know if you have any ideas how this could be done.

@chrisguttandin

This comment has been minimized.

Copy link
Owner

commented May 4, 2019

Hi @TwitchBronBron,

I finally found a way to set up automated tests for this problem. You can find the tests here: https://github.com/chrisguttandin/standardized-audio-context/blob/master/test/integration/memory.js

I used puppeteer which means, the tests run currently in Chrome only. That's not optimal, but I don't know of a way to do the same thing in other browsers.

To verify the memory usage I don't inspect the heapSize anymore. I do instead use the queryObjects() function which counts the instances of the given object. I count the instances of Object.prototype in memory which basically covers every JavaScript object. All tests verify that the total number of objects does not change.

I tried various other methods as well. I plan to write a blog post about it and I will update this issue with a link when I got the time to actually do it.

Please note that the update is not yet published on npm. It will be part of the next major release which will be v20.

Many thanks again for reporting this issue. It was very helpful.

@TwitchBronBron

This comment has been minimized.

Copy link
Author

commented May 4, 2019

Awesome! Glad you were able to get to the bottom of this. Thanks for the very detailed explanations as well!

@chrisguttandin

This comment has been minimized.

Copy link
Owner

commented Jun 8, 2019

Hi @TwitchBronBron, I finally published an article about the journey that led me to the final solution. Here is the link: https://media-codings.com/articles/automatically-detect-memory-leaks-with-puppeteer.

Do you want to be mentioned in the article. I would be very happy to add your name and a link to your website or Twitter profile somewhere because you are the one that made me explore all those things. I just didn't do it so far because I wasn't sure if you would want that. Just ping me if you don't mind to be mentioned there.

@TwitchBronBron

This comment has been minimized.

Copy link
Author

commented Jun 8, 2019

Hey, sure, I would love the mention! https://bronley.com (there's nothing much there, but it has all of my links to other things)

@chrisguttandin

This comment has been minimized.

Copy link
Owner

commented Jun 8, 2019

Alright, I edited the text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.