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

Improve the documentation of the memory block size growth feature #263

Merged
merged 2 commits into from
Feb 4, 2025

Conversation

nical
Copy link
Contributor

@nical nical commented Jan 31, 2025

Followups from #254. cc @MarijnS95

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Thanks, that's quick! I think there's one comment that you can still address: #254 (comment). For the others, I marked most as resolved and the few remaining ones require larger refactors that protrude the scope of the original PR.

@nical nical force-pushed the grow-followups branch 3 times, most recently from 86a9379 to 889ba07 Compare January 31, 2025 13:07
src/lib.rs Outdated
/// Defaults to 256MB.
min_device_memblock_size: u64,
/// The size of device memory blocks doubles each time a new allocation is needed, up to
/// `device_maximum_memblock_size`.
/// The maximum size for shared device memory blocks.
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that for min_device_memblock_size you say "GPU only memory type", here you say "shared". That probably doesn't mean "shared with CPU" but "shared with multiple smaller allocations", right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I meant shared as opposed to dedicated blocks which hold a single allocation and are not limited to max_*_memory_block_size. Let me know if I should call it something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to make the wording more consistent between the min and max docs, remove "shared" there and drop a note about these limits applying only to shared-as-in-non-dedicated blocks in the struct-level doc.

@nical
Copy link
Contributor Author

nical commented Feb 3, 2025

Apologies for the many typos and awkward wording. It looks like in the end I didn't save you much time by submitting the PR myself.

@MarijnS95
Copy link
Member

No worries, I would have made the same typos and weird wording myself, you just get blind to it when constantly shifting code around. It always helps to have a second pair of eyes :)

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +290 to +293
/// Note that these limits only apply to shared memory blocks that can hold multiple allocations.
/// If an allocation does not fit within the corresponding maximum block size, it will be placed
/// in a dedicated memory block holding only this allocation, without limitations other than what
/// the underlying hardware and driver are able to provide.
Copy link
Member

Choose a reason for hiding this comment

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

Not something to fix in this PR, nor to mention in the documentation, but I think there's currently a mistake where we allocate such a "dedicated memory block" directly also if the minimum size didn't yet grow to the maximum size:

let memblock_size = allocation_sizes.get_memblock_size(is_host, self.active_general_blocks);
let size = desc.requirements.size;
let alignment = desc.requirements.alignment;
let dedicated_allocation = desc.allocation_scheme != AllocationScheme::GpuAllocatorManaged;
let requires_personal_block = size > memblock_size;

But fixing that in whichever way (i.e. let requires_personal_block = size > allocation_sizes.max_xxx_memblock_size;) might only make the code more confusing, as we'd have to either immediately allocate all blocks up until a block that would fit, or find the smallest multiple between min..max where that allocation would fit (that's a shared and not dedicated block).

@MarijnS95 MarijnS95 merged commit 955b13a into Traverse-Research:main Feb 4, 2025
13 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.

3 participants