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

Dynamically growing scratch allocator & exposed image memory type #68

Merged
merged 7 commits into from
Aug 2, 2023

Conversation

jedjoud10
Copy link
Contributor

Should fix both #66 and #38

I shoulda split it into two branches tbh.

@jedjoud10 jedjoud10 changed the title Dynamically growing scratch allocator & exposed imaged memory type Dynamically growing scratch allocator & exposed image memory type Aug 1, 2023
} else {
Err(anyhow::Error::from(Error::UnmappableBuffer))
if chunk_size % alignment != 0 {
anyhow::bail!("Chunk size must be a multiple of alignment");
Copy link
Owner

Choose a reason for hiding this comment

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

Document this requirement in the function docs

buffers: Vec::new(),
local_offset: 0,
chunk_size,
current_buffer: None,
Copy link
Owner

Choose a reason for hiding this comment

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

To avoid hitches on the first draw it might make sense to allocate a single chunk on construction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. What should be the size of said initial chunk though? Or should it be used configured

Copy link
Owner

Choose a reason for hiding this comment

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

Just one chunk of chunk_size bytes no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would work yep. I guess it would create a dedicated buffer for allocations larger than chunk_size, and not use the default init one

let size: u64 = size.into();

// Round up to the preferred alignment value
let padded_size = ((size as f32) / (256 as f32)).ceil() as u64 * 256;
Copy link
Owner

Choose a reason for hiding this comment

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

Should this 256 be hardcoded here instead of using the stored alignment value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stored alignment value. I forgor

let whole_buffer_size = size.max(self.chunk_size);
let buffer = Buffer::new(self.device.clone(), &mut self.allocator, whole_buffer_size, MemoryType::CpuToGpu)?;
if !buffer.is_mapped() {
return Err(anyhow::Error::from(Error::UnmappableBuffer));
Copy link
Owner

Choose a reason for hiding this comment

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

bail!

@NotAPenguin0
Copy link
Owner

NotAPenguin0 commented Aug 1, 2023

I'd also add a method to shrink a scratch allocator to a given size. This should be similar to reset() and be unsafe and should only be called when the memory is safe to re-use probably.

@jedjoud10
Copy link
Contributor Author

How would that work though? Since the allocator itself contains multiple small allocations of variable size (unless you'd want them to always be of size chunk_size)

@NotAPenguin0
Copy link
Owner

I figured reset would combine all current buffers into a larger one, but that might not be necessary

@NotAPenguin0 NotAPenguin0 merged commit a775036 into NotAPenguin0:master Aug 2, 2023
4 checks passed
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.

Dynamically growing scratch memory Allow for configuring memory location on images
2 participants