Memory leak with Engine.loadAudioFilesIntoPool

[d.healey]

Well I didn't find a solution but I have confirmed the issue. You can demonstrate it with a standalone build, no need to have a DAW in the equation.

@Christoph-Hart You should probably take a look at this. Here's the project as above but I added some IRs.

convolution-test.zip

[AbidextrousMoose]

I fixed it on our end.

There are three things at work here, one on the C++ side and two on the Editor side.

The two separations are dependent on whether the action is question is done in the JS Layer or not.

1. If the Convolution IR is not a UI element that has savedInPreset set to true if any other UI element is and also targets the Convolution IR as a function of its parameter target, it will leak memory.
2. If the combo box selection (as demonstrated in both examples above) is done via the code as it's written, you will leak memory. However this also happens in the case where you manually select the IR via the file browser. I've identified as the culprit of this the way we deallocate the Convolution objects. They are queued for deletion in ConvolutionBase.cpp on line 673 and 674 for both channels. However, they never leave the deletion queue, so the copys that get sent there never run their destructors. This may be an issue with the deletion queue itself, however, I have changed those lines to use the reset() function of the convolution itself and that seems to solve the issue in the near term. Though I don't know the consequences of not queueing the deletion like @Christoph-Hart would.

        if(convolverL != nullptr)
        {
            convolverL.reset();
            convolverR.reset();
        }
        
        convolverL = s1;
        convolverR = s2;

3. In the event that number 1 is active, any preset change will leak memory. Whether through the PresetBrowser or not it will always leak. So if say in FL Studio the Instrument or Plugin is reinitialized or the preset changed and reloaded, it will always leak. We fixed all of this by including the ConvolutionIR itself in the preset.

That's all for now. I don't think it wise to make a PR with the above changes as I don't 100% trust my solution using reset() but it works in our testing and we'll be releasing an update to our primary library (all the plugins of which use this) today.

I'll let you know our results. Would be thrilled to hear any additional opinions.

Edit: loadAudioFilesIntoPool() is a red herring. I wouldn't bother looking into that. The issue is with the Convolution itself.

[tomekslesicki]

@AbidextrousMoose thanks for a detailed report!

We fixed all of this by including the ConvolutionIR itself in the preset.

Do you mean by including an Audio Waveform UI element and pointing it to the IR slot?

@Christoph-Hart did you have a chance to look into it?

[tomekslesicki]

@Christoph-Hart could you please look into this? Thank you!

[Christoph Hart]

Weird how I haven‘t noticed that - both the topic and the bug. Just from looking at the code it might be the case that the leaks occur only if background processing of the convolution effect is disabled, but then again I‘ve always have that enabled now.

Can you verify that? The fix with calling reset() is not the best option, there is a reason why I had to defer the deletion to the background thread (I vaguely remember it being added to fix occasional crashes).

[tomekslesicki]

@Christoph-Hart if you mean the Multithread option, have it enabled, and the bug is still there

[Christoph Hart]

@tomekslesicki alright, then I'll take a look, but it's weird that I never noticed that as I'm shipping quite a few plugins with convolution reverbs and never noticed any leaks / memory increase.

[Christoph Hart]

@Christoph-Hart I can confirm that if the multithreading option is disabled (like it is in the test project), the function that cleans up the pending impulse responses is not called periodically, but only when you close the plugin, so while it's technically not a memory leak, it certainly accumulates unnecessary resources.

This was an easy fix - not calling reset directly but making sure that the cleanup process is executed independant of the multithreading flag.

Once you enable the multithreading option, the problem goes away though - even if you deactivate the option again, as the thread keeps running so the cleanup will be performed even if the actual rendering is moved back to the audio thread.

@tomekslesicki can you check this with the test project that was uploaded earlier?

You even don't need to compile it, just look at the task manager in Windows should be enough to notice the leaks:

1. Download the project, drop in any IR files
2. Load the project
3. Open the task manager and notice the memory usage
4. Smash that combobox to load different IRs
5. Notice how the number goes up.
6. Now open the convolution module and enable background processing.
7. The memory usage immediately drops to the original value because the old convolution objects are cleaned up at the first thread execution loop.
8. Keep smashing, but now the memory usage stays down, even if you deactivate the multithreading again.

I've committed a fix so now it should clean up properly without the background thread needing to be enabled.

@tomekslesicki if your project is still exhibiting the problem we need to analyse what differs to this test project, eg:

• are you using the HISE convolution module or the convolution node?
• are you using multiple convolution modules? Have all of them set Multithreading on?
• Is the memory increase more or less proportional to the file size of the impulse responses?

[tomekslesicki]

@Christoph-Hart that seems to have fixed it on my end, thank you!

[tomekslesicki]

@Christoph-Hart well, I might have been too fast! It works on Windows, but I can't compile the network on a Mac

[Christoph Hart]

@tomekslesicki yup that is a known issue from one of my last commits. I fixed that now - can you check if the DLL compiles on macOS now?