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

Remove NotePlayHandleManager #7338

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Jun 22, 2024

Removes NotePlayHandleManager. This was implemented for memory savings when allocating NotePlayHandles, but with enough of them, this class will start just allocating more space rapidly anyways, so we might as well just use new for now and keep things simple. This alone justifies its removal, but it will be good if we can cut down on some code in general. Also has a memory leak that may not be worth worrying about fixing.

Ideally, we would not design the audio processing to be based upon handles that have be dynamically allocated on the audio thread. This design is fundamentally flawed as we want to avoid heap allocations in a real-time context. However, it may be worthwhile to set up a general memory pool in the future instead as a temporary solution.

@messmerd
Copy link
Member

messmerd commented Jun 23, 2024

It looks like NotePlayHandleManager is an arena allocation implementation for note play handles. From what I can tell, removing it would make audio threads less efficient and less realtime safe by requiring memory allocations for every note play handle rather than acquiring the memory from the preallocated pool.

I don't see what good removing it would do if it's not being replaced with something better.

@Veratil
Copy link
Contributor

Veratil commented Jun 23, 2024

See #1088 for history of it being added. There was some for and against it.

@sakertooth
Copy link
Contributor Author

sakertooth commented Jun 23, 2024

It looks like NotePlayHandleManager is an arena allocation implementation for note play handles. From what I can tell, removing it would make audio threads less efficient and less realtime safe by requiring memory allocations for every note play handle rather than acquiring the memory from the preallocated pool.
I don't see what good removing it would do if it's not being replaced with something better.

Ideally, we would pre allocate all the memory needed before processing the audio. Perhaps Note's could contain a NotePlayHandle somehow and function as expected that way. I'll see if I can get a replacement soon.

To clarify, NotePlayHandleManager could possibly allocate more data on calls to NotePlayHandleManager::acquire to extend the size of its memory pool, so it might be common for NotePlayHandleManager to just be allocating from the heap on the real time audio threads anyway, given that there are enough notes.

@Rossmaxx Rossmaxx mentioned this pull request Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants