LMMS
Reduce Memory Leaks
#6549 tried to fix the same leak but that got into merge conflicts because we removed our inbuilt memory management in favour of the STL mechanisms.
I think we should spend a little more time and refactor the NotePlayHandleManager class. Maybe use std::vector for storage.
I think we should spend a little more time and refactor the NotePlayHandleManager class. Maybe use std::vector for storage.
I would like for us to move to a better design. NotePlayHandleManager is poorly implemented (IMO), and it would probably be best to move to something better altogether, rather than to try and fix it up.
The idea of having the handles be allocated on the heap as the heart of our audio processing is flawed on a fundamental level. I was thinking of designing this in such a way that the handles are pre allocated and stored in sample clips, a note, instruments, etc, and the audio processing would use those handles directly somehow.
This would require a large refactoring however, so it might be worthwhile to just create a realtime safe memory pool that can be used with allocations on an audio thread. That has its own problems (e.g., what should the optimal size be for this pool?), but since we have some kind of memory pool as of right now, making a better one is the easiest solution.
Regarding the optimal memory pool size, it seems like we have some kind of limit already for the maximum number of play handles (JOB_QUEUE_SIZE is 8192), so we could just take the handle with the largest size in bytes, multiply it by 8192, maybe do some rounding/added bytes, and that I think would be an "okay" size. This is probably not the best way of doing it, but it could work well enough.
I would like for us to move to a better design.
NotePlayHandleManageris poorly implemented (IMO), and it would probably be best to move to something better altogether, rather than to try and fix it up.
This is way out of my skill level, I don't even want to attempt it lmfao
@sakertooth @messmerd I do agree we must rework NotePlayHandleManager but can we get done with this one leak first. @AW1534 is a new contributor and I don't feel they will be comfortable with a rework. Let's take the work off their shoulders and do the rework at #7338 , that said, I think merging this would be a good idea for till the rework lands in master.
@sakertooth @messmerd I do agree we must rework NotePlayHandleManager but can we get done with this one leak first. @AW1534 is a new contributor and I don't feel they will be comfortable with a rework. Let's take the work off their shoulders and do the rework at https://github.com/LMMS/lmms/pull/7338 , that said, I think merging this would be a good idea for till the rework lands in master.
I didn't say anything about @AW1534 needing to do the rework, I just gave general ideas for what we should move towards in the future. As for the PR, we don't need to check if its nullptr or not when deleting.
From cppreference:
In all cases, if ptr is a null pointer, the standard library deallocation functions do nothing.
I think its fine to just add delete s_available[0] (i.e, your std::free(s_available[0]) line without all the if nullptr checks and using delete instead).
Also, this deletion should happen before delete[] s_available since that pointer was assigned into the array (which is already being done, but I just wanted to be clear).
Much better, but I think the comments aren't need here IMO. We should get in the habit of using these judiciously. s_available = nullptr; might not be needed too, but since this isn't a destructor, I think it's fine to keep.
The idea of having the handles be allocated on the heap as the heart of our audio processing is flawed on a fundamental level. I was thinking of designing this in such a way that the handles are pre allocated and stored in sample clips, a note, instruments, etc, and the audio processing would use those handles directly somehow.
This sounds more complex than maintaining a pool. What about arpeggios and note stacking? The chord and range of those can be automated, so we'd either need to track the automation to know the maximum value of each, or allocate enough handles per note to cover all possible settings (which would be quite a large number). And once we eventually support third-party MIDI effects, this task becomes effectively impossible.
Maybe use
std::vectorfor storage.
This works if we want a fixed-size pool, but not so well for a dynamic one - pointers to handles will be invalidated when the vector is resized.
This sounds more complex than maintaining a pool. What about arpeggios and note stacking? The chord and range of those can be automated, so we'd either need to track the automation to know the maximum value of each, or allocate enough handles per note to cover all possible settings (which would be quite a large number). And once we eventually support third-party MIDI effects, this task becomes effectively impossible.
Fair, but it still raises the question as to if allocating play handles as how audio is being processed is a good design. The allocations of course can be made real-time safe through a memory pool, so it's not much of a problem really, but I thought of other designs such as one based on pre-allocated audio nodes and a dependency graph and wondered if that'll make things easier/better (this would require a huge refactor though, so if it isn't necessarily a better design, then there's no point).
I believe the data structure we'd want would be a "hive" or "colony" like the one proposed for C++26.
It's an unordered container which ensures stable memory addresses for elements in each allocated block. Each element has a boolean to mark whether that element is active or empty/erased. When iterating through the container, empty/erased elements are skipped. When a block is full of active elements, another block is allocated the next time an element is inserted and the old block points to the new block. Since there's no reallocation, no pointers or references are invalidated.
I think a hive could be approximated with this underlying data structure:
std::forward_list<std::vector<std::optional<T>>>
I was thinking more along the lines of using std::pmr::* and making it a per-thread memory pool so the pool can be in real-time.
I thought of other designs such as one based on pre-allocated audio nodes and a dependency graph and wondered if that'll make things easier/better
That's certainly a good idea, but it's orthogonal to the issue of how we allocate memory to track per-note data. The number of notes can't be predicted in advance, as I explained above, so while we can pre-allocate memory for effects and per-instrument data, and arrange these in a dependency graph, we still need to handle notes differently. Using entire instruments, rather than notes, as the job unit for the mixer and giving each instrument a maximum polyphony would work, but that's just a fixed memory pool by another name.