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

Use thread local memory pools when allocating jobs #7338

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

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Jun 22, 2024

Removes NotePlayHandleManager and uses thread local memory pools for all ThreadableJob's. The memory pool in question is actually std::pmr::unsynchronized_pool_resource. They were made thread local to avoid synchronization.

@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
@messmerd
Copy link
Member

messmerd commented Nov 20, 2024

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
Copy link
Contributor Author

sakertooth commented Nov 20, 2024

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
Copy link
Contributor Author

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 sakertooth changed the title Remove NotePlayHandleManager Remove NotePlayHandleManager and use std::pmr::unsynchronized_pool_resource for all ThreadableJob's Nov 21, 2024
@sakertooth sakertooth changed the title Remove NotePlayHandleManager and use std::pmr::unsynchronized_pool_resource for all ThreadableJob's Use thread local memory pools when allocating jobss Nov 21, 2024
@sakertooth sakertooth changed the title Use thread local memory pools when allocating jobss Use thread local memory pools when allocating jobs Nov 21, 2024
@sakertooth
Copy link
Contributor Author

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.

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.

3 participants