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

Make command buffer/descriptor set allocators Sync again #2046

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

marc0246
Copy link
Contributor

Made StandardCommandBufferAllocator and StandardDescriptorSetAllocator thread-safe using TLS.

At first I wanted to implement the TLS myself, using a simple RwLock<HashMap<ThreadId, T>>, but I noticed that Windows kept getting into deadlocks with that for some reason. So instead I opted for an established library, thread_local, which was added to the private dependencies. It's a very light-weight library and also more optimized than what I had in mind. For one it doesn't need acquiring any read locks, and also the entries in the TLS can be reused. This would not have been the case with what I have tried, as threads would always drop their allocator on exit and when a new thread would allocate it would create a brand new allocator for itself. This way those reentrant threads can reuse the allocators.

Another good thing that came from trying this library is that after benchmarking, there's almost no overhead to using this TLS over none at all. It's 6ns of overhead on my CPU, granted I also have a very dated CPU. Therefore, I think it makes no sense to have a non-Sync version at all, if we're going to use this library. The only performance implication this has is when a lot of threads need to access the TLS concurrently, which would lead to CPU-level contention on the AtomicPtrs that the TLS uses. But even with unreasonably high contention that I tested it with (all of my threads going at it) the times for allocation only doubled. If this is an issue the user still has the option to quite simply not share the allocators between threads.

@Rua
Copy link
Contributor

Rua commented Oct 27, 2022

That library is a nice find! Maybe it will come in handy for other parts of Vulkano too.

Everything looks good, I'll merge when the typos are fixed.

@marc0246
Copy link
Contributor Author

Thanks for the corrections! Fixed.

@Rua Rua merged commit 8a1c91f into vulkano-rs:master Oct 27, 2022
@marc0246 marc0246 deleted the tls branch October 27, 2022 19:25
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.

2 participants