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

Switch to a more expressive way to specify memory usage #2264

Merged
merged 7 commits into from
Jul 19, 2023

Conversation

marc0246
Copy link
Contributor

@marc0246 marc0246 commented Jul 16, 2023

Previously there were these memory usages:

MemoryUsage Ideal memory type
DeviceOnly DEVICE_LOCAL
Upload DEVICE_LOCAL | HOST_VISIBLE | HOST_COHERENT
Download HOST_VISIBLE | HOST_CACHED

However, almost all PC and laptop implementations also have a memory type that is HOST_VISIBLE | HOST_COHERENT, which was not possible to request using those, which is the biggest reason for this change. However, some other implementations have more quirky memory types as well, such a DEVICE_LOCAL | HOST_VISIBLE | HOST_CACHED memory type on MacOS and mobile devices. It was also not possible to distinguish between that and the non-device-local host-cached one.

Here's all the combinations possible now, using the presets available as associated constants of MemoryTypeFilter:

MemoryTypeFilter Ideal memory type
PREFER_DEVICE DEVICE_LOCAL
PREFER_DEVICE | HOST_SEQUENTIAL_WRITE DEVICE_LOCAL | HOST_VISIBLE | HOST_COHERENT
PREFER_DEVICE | HOST_RANDOM_ACCESS DEVICE_LOCAL | HOST_VISIBLE | HOST_CACHED
PREFER_HOST
PREFER_HOST | HOST_SEQUENTIAL_WRITE HOST_VISIBLE | HOST_COHERENT
PREFER_HOST | HOST_RANDOM_ACCESS HOST_VISIBLE | HOST_CACHED

There are of course still some memory types that can't be differentiated, such as in the case of MemoryTypeFilter::HOST_RANDOM_ACCESS, there might be both HOST_VISIBLE | HOST_CACHED and HOST_VISIBLE | HOST_COHERENT | HOST_CACHED around. If the user truly needs something that specialized, then they should specify the memory type filter manually.

Changelog:

### Breaking changes
Changes to memory allocation:
- `AllocationCreateInfo::usage` and `SubbufferAllocatorCreateInfo::memory_usage` were replaced by a `memory_type_filter` field, to allow for a more flexible selection of the memory type. Additionally, `SubbufferAllocatorCreateInfo::memory_type_filter` defaults to `MemoryTypeFilter::PREFER_DEVICE` for consistency with `AllocationCreateInfo`, unlike the previous default of `MemoryUsage::Upload`.
- `SubbufferAllocatorCreateInfo::buffer_usage` is now empty by default for consistency with `BufferCreateInfo`.

}
HostAccessType::RandomAccess => {
filter.required_flags |=
MemoryPropertyFlags::HOST_VISIBLE | MemoryPropertyFlags::HOST_CACHED;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should HOST_CACHED be a strict requirement here?

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 don't like it either, but I think it's needed because of this scenario:

  • DEVICE_LOCAL
  • DEVICE_LOCAL | HOST_VISIBLE | HOST_COHERENT
  • HOST_VISIBLE | HOST_COHERENT
  • HOST_VISIBLE | HOST_COHERENT | HOST_CACHED

Here, preferring DEVICE_LOCAL and HOST_CACHED would result in a tie, and you would get the memory type at index 1 which is, quite literally, the worst possible choice here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not every device is required to have HOST_CACHED memory, so asking for RandomAccess would fail on such a device, which is bad.

The problem I also see is that you're treating both flags as equally important, when they aren't necessarily. Is the caching more important, or being on the device?

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 aware that it's not required by the spec, however in practice they all have it. Maybe we should ask the VMA guys why they thought its ok?

The problem I also see is that you're treating both flags as equally important, when they aren't necessarily.

Well yes, that is an inherent problem coming from the fact that MemoryTypeFilter::preferred_flags is just a list of flags, no weights.

@marc0246
Copy link
Contributor Author

So I thought about it some more, and since I plan on adding a AllocationCreateInfo::memory_type_filter field, that would then again leave us with 2 ways to specify the same thing. The allocator would turn the MemoryLocationPreference and HostAccessType into a MemoryTypeFilter and OR it together with AllocationCreateInfo::memory_type_filter.

This might be a bit hard to understand for the user, which is why I think we could simplify this even more and only have the memory_type_filter, with presets as associated constants of MemoryTypeFilter, and with it implementing BitOr. Another benefit would be that you only need to import one type. The downside would be that we can't ever do more complex branching on the MemoryLocationPreference and HostAccessType inside the allocator in the future.

@marc0246
Copy link
Contributor Author

How does this look?

AllocationCreateInfo {
    memory_type_filter: MemoryTypeFilter::PREFER_HOST | MemoryTypeFilter::HOST_SEQUENTIAL_WRITE,
    ..Default::default()
}

Or maybe even

AllocationCreateInfo {
    memory_type_filter: MemoryTypeFilter::STAGING,
    ..Default::default()
}

I would prefer not to complicate things at the possible prospect of doing more black magic inside the allocator. I don't think that's that useful anyway. If we ever need to create a MemoryTypeFilter from some parameters, we can just make that an associated function of the type. Easier for the compiler because it doesn't need to inline all the logic inside the allocator itself, easier for someone implementing an allocator, and easier for a user to understand the logic.

@Rua
Copy link
Contributor

Rua commented Jul 17, 2023

Nice, I like it! Using Rust features in ways that don't work for a C API!

@Rua
Copy link
Contributor

Rua commented Jul 19, 2023

Everything good, thanks!

@Rua Rua merged commit 2c00670 into vulkano-rs:master Jul 19, 2023
3 checks passed
Rua added a commit that referenced this pull request Jul 19, 2023
@marc0246 marc0246 deleted the memory-usage branch July 19, 2023 18:31
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
)

* Switch to a more expressive way to specify memory usage

* Fix tests

* Fix examples

* Forgot to adjust `SubbufferAllocatorCreateInfo::location_preference`s

* Oopsie

* Redo all of it

* Oopsie
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
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