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

Reuse cached fences Vec to reduce allocations in draw call to zero #2057

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

Conversation

nikarh
Copy link
Contributor

@nikarh nikarh commented May 5, 2023

This reduces heap allocations to zero (at least in my use case). Related to #2056.

The idea here is that Context and CommandContext can only be used in a single thread, and fences Vec's used to be created and collected in the scope of a function call. Meaning we can pre-allocate a single Vec for fences, and then just "take" it on every call and "return it back" when the call ends.

There are a couple of caveats though:

  1. Inserter has a lifetime. If we are to cache the Vec of Inserters, the lifetime in our cache must be broader than the lifetime that Inserter will have in a particular draw/compute call. Meaning I don't really see a way of implementing that without the unsafe magic of downcasting lifetimes. So the general idea is:
    1. We have a preallocated Vec somewhere in the context. The size of the Vec is zero, but the capacity is remaining from the previous call
    2. In a draw/compute call we take a mut ref to this Vec, downcast a lifetime to a local one, and push new inserted to this vec as needed
    3. At the end of the draw/compute call we fulfill those fences, by taking them from Vec
    4. We clean the Vec, so the size now is 0 (but capacity remains), so the Vec does not contain any references with a local lifetime
    5. We're back to an empty Vec of inserters with a 'static lifetime (but capacity remains, so we don't have to reallocate in the next draw/compute calls)
  2. Due to the current architecture I wasn't able to put this fences cache in CommandContext without refactoring Fences and sync stuff, simply because Inserters.insert call takes a mut ref to CommandContext as an argument. So in this implementation, I made a pub(crate) fn that "takes" fences Vec from the Context instead.

@nikarh nikarh changed the title Reuse cached fences Vec to reduce allocations to zero Reuse cached fences Vec to reduce allocations in draw call to zero May 7, 2023
@Melchizedek6809
Copy link
Member

Thank you very much, this sounds great!

Sorry for not replying sooner, I'm currently moving and therefore a bit short on time. Should be over next week though, then I can take a closer look at this PR :)

Copy link
Member

@Melchizedek6809 Melchizedek6809 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite good! Ran most examples without issues and a Game of mine also runs without issues with these changes, I only have a couple of minor questions/nitpicks.

src/buffer/fences.rs Outdated Show resolved Hide resolved
src/buffer/fences.rs Outdated Show resolved Hide resolved
@nikarh
Copy link
Contributor Author

nikarh commented Jun 22, 2023

Oh, also, now that I think about it, it's possible to use SmallVec with some sensible constant size for fences instead of all that. This would prevent us from doing the ugly stuff (transmute).
But this won't guarantee always 0 allocations, because if more fences are required, SmallVec would spill into a Vec, leading back to the same issue.

@nikarh
Copy link
Contributor Author

nikarh commented Jul 2, 2023

There's also another approach that doesn't involve writing unsafe code.
Considering that all of the allocations have a lifetime of some logical unit of work (like a draw call), finite allocations happen in them, and gl is single-threaded, it's possible to use an arena allocator instead of the global one, like bumpalo.

The core idea here is that the arena is stored in the context, and the actual memory used by it is never freed, but after the e.g. draw call it is reset, invalidating all allocations in the arena.

But that would introduce a new fairly complex dependency, and in that case, it makes sense to do a much broader refactoring to basically do all of the allocations in the arena, like replacing SmallVec's with it (which would solve allocations when it spills)

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

2 participants