Skip to content

Add SimpleVramAllocator::free_all and attach a lifetime to VRAM chunks #119

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

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

ayrtonm
Copy link
Contributor

@ayrtonm ayrtonm commented Jun 6, 2022

This allows the borrow checker to work with VRAM allocations like I mentioned on discord. I don't think it's super-useful with the current sceGu API, but it doesn't seem any more cumbersome to use which is what I was mainly concerned about. The issue with sceGu is that it takes pointers as arguments so the VRAM chunk lifetime provides no benefit in this particular case

// Borrow the allocator for 'a, get a VRAMChunk<'a> then get a pointer to the
// chunk and drop the VRAMChunk<'a>
let chunk_ptr = allocator.alloc(size).as_mut_ptr();

// Mutably borrowing the allocator for 'b is fine since VRAMChunk<'a> was
// already dropped
allocator.free_all();

// Allocates another chunk overlapping the first one. This is allowed because
// chunk_ptr doesn't have a lifetime and VRAMChunk<'a> was dropped.
let new_chunk = allocator.alloc(size);

// Use-after-free
sceGuDrawBuffer(chunk_ptr);

For this to be useful, you'd have to keep the VRAMChunk<'a> until it's passed to sceGuDrawBuffer like in the PR example. Or we could make a sceGu API that accepts VRAMChunks and calculates the pointer internally.

Also the AtomicU32 in the allocator is just for interior mutability (could've been Cell or RefCell). It doesn't inherently make the allocator thread-safe, but the demos use a singleton anyway so it doesn't matter.

@ayrtonm
Copy link
Contributor Author

ayrtonm commented Jun 6, 2022

Also @MrAwesome @overdrivenpotato do you remember why the dealloc methods were dropped before #60 was merged? I imagine no one's bumping into the 8MB VRAM limit so it's probably not that useful, but I couldn't find an explanation on that PR.

@overdrivenpotato
Copy link
Owner

The realloc and dealloc methods in that PR were just stubs with an unimplemented!() body.

@ayrtonm
Copy link
Contributor Author

ayrtonm commented Jun 7, 2022

I should probably remove the comments in examples/cube, but there's no reason this needs to be a draft.

@ayrtonm ayrtonm marked this pull request as ready for review June 7, 2022 19:59
@overdrivenpotato overdrivenpotato merged commit 27736ee into overdrivenpotato:master Jun 7, 2022
Copy link
Contributor

@zetanumbers zetanumbers left a comment

Choose a reason for hiding this comment

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

I will make a pr to address my issues

Comment on lines +91 to +93
let old_offset = self.offset.load(Ordering::Relaxed);
let new_offset = old_offset + size;
self.offset.store(new_offset, Ordering::Relaxed);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a good old race condition right there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional btw? Perhaps this was written as a singlethreaded code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SimpleVramAllocator is used as a singleton, so I was treating this like singlethreaded code. The AtomicU32 is only for interior mutability and I went with that over Cell to make it easier in case anyone wants to try using the allocator from multiple threads (though that would require a few more changes).

Copy link
Contributor

Choose a reason for hiding this comment

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

But the problem is SimpleVramAllocator implements Sync, which means you can share references of it between different threads, and function in question (alloc) requires just a shared reference, so alloc can be simultaneously run on multiple different threads. in that case alloc can return two aliasing mem chunks.

Copy link
Contributor Author

@ayrtonm ayrtonm Jun 14, 2022

Choose a reason for hiding this comment

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

In bumpalo thread support is provided by a "herd" which is roughly a Mutex<Vec<Bump>> that creates a new bump allocator with its own pool of memory for each thread. Splitting VRAM per thread seems impractical and I don't think there's a safe way to use one bump allocator from multiple threads. So I'd say let's just add impl !Sync for SimpleVramAllocator in #120 for now.

Copy link
Contributor

@zetanumbers zetanumbers Jun 14, 2022

Choose a reason for hiding this comment

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

let's just add impl !Sync for SimpleVramAllocator

No, i think have figured atomics out. It's just nvidia-driver-390 killed my os, so i am on windows for now, that takes it this long and i am back on linux.

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