Remove `NotePlayHandleManager`

[sakertooth]

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]

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]

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

[sakertooth]

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.

[messmerd]

With the recent macOS CI update, we should now be able to use everything from C++17's <memory_resource> header, and it looks like std::pmr::synchronized_pool_resource may be exactly what we want.

EDIT: Unfortunately, it looks like the implementation uses std::shared_mutex, so it isn't lock-free. It would still probably be an improvement over NotePlayHandleManager though.

If there was a way to guarantee accesses came from a single thread, we could use std::pmr::unsynchronized_pool_resource which is real-time safe.

[sakertooth]

If there was a way to guarantee accesses came from a single thread, we could use std::pmr::unsynchronized_pool_resource which is real-time safe.

One way is to use a thread_local std::pmr::unsynchronized_pool_resource. However, if we strictly do not want to dynamically allocate, it might be a bit difficult because using a null upstream memory resource can potentially lead to OOM issues and crash the app. I've tested this in my own personal branch (granted the project I tested with had ~36K+ notes).

My idea is to still use a thread_local std::pmr::unsynchronized_pool_resource with dynamic allocation as an upstream memory resource. Given a decent size we give to std::pmr::monotonic_buffer_resource, I think this will still be a major improvement over NotePlayHandleManager. I also think we should generalize this pool to at least all the handles somehow, not just NotePlayHandle.

I'll try to do some work towards this soon.

~...also minor rant but I really dont like the idea of play handles. I think I mentioned this previously, but even things like moving the playhead ruins playback if you dont start at the beginning of each note because the handles are only made when the playhead first encounters them. I think its still okay to go forward with the memory pool idea though, this is an issue we can fix much later.~ (I'm not sure how big of an issue this really is)

[sakertooth]

Hey @messmerd, I went ahead and made changes that make allocations involving all types of ThreadableJob's use a thread_local std::pmr::unsynchronized_pool_resource. One thing I immediately noticed from this change is that the Greppi demo loads a bit faster.

[sakertooth]

Nevermind, the Greppi demo loading time is about the same now, both fast really before and after the changes. Maybe it was just a fluke on my part.

[sakertooth]

Do we still want this?