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

mr::resource concept should require const allocate and noexcept deallocate #2411

Closed
ericniebler opened this issue Sep 12, 2024 · 4 comments
Closed
Labels
bug Something isn't working right. CUDA Next Feature intended for the Cuda Next experimental library

Comments

@ericniebler
Copy link
Collaborator

const allocate

memory resources have allocate and deallocate functions, and no observers of any mutable state. that makes them (logically) immutable. allocate and deallocate do not change any observable mutable state, so they should be const

this matters because (a) many memory resources are immovable and/or non-copyable, and (b) the type-erasing any_resource would like to share ownership of the instance it wraps (since it cannot assume copyability). immutable objects are safely shareable without breaking value semantics.

requiring that allocate and deallocate are const-qualified will encourage memory resource authors to make their memory resources thread-safe, which is necessary if instances are to be shared.

noexcept deallocate

deallocate routines are often called from destructors. destructors shouldn't throw. an error during deallocation should assert in debug builds. it's debatable whether such an error should terminate the process or be silently ignored in release builds. either way, throwing is the wrong thing to do. i don't know of any code[*] that would correctly handle an exception thrown from a destructor.

i would specifically like feedback from @harrism since this would require changes to RMM's memory resources, and from @miscco who wrote the resource concept.

[*]: outside of very specific and carefully coded RAII utilities like scope guards

@ericniebler ericniebler added bug Something isn't working right. CUDA Next Feature intended for the Cuda Next experimental library labels Sep 12, 2024
@jrhemstad
Copy link
Collaborator

Making deallocate noexcept makes sense.

I'm still working my way through wrapping my head around the type design theory of making allocate const.

What I'm stuck on is that a memory pool will definitely have internal mutable state, and so making allocate const will mean having to add 'mutable' to that state.

The end result is effectively the same, so what did making allocate const buy me?

@miscco
Copy link
Collaborator

miscco commented Sep 16, 2024

I aggree with the noexcept deallocate, but const allocate does not sound like a valid choice.

Assume you have any pool-allocator. That will need to store internal data on allocate. Forcing the user to make this method const will lead to all sorts of UB in the implementation that I am not comfortable with

@harrism
Copy link
Contributor

harrism commented Sep 16, 2024

I (also) agree with the noexcept deallocate conjecture.

I think pool state could be considered nonobservable, so perhaps a pool MR is not a perfect counterexample for the const allocate conjecture. But I think RMM's statistics_resource_adaptor is a pretty solid counterexample, where allocate and deallocate modify observable state:

https://github.com/rapidsai/rmm/blob/branch-24.10/include/rmm/mr/device/statistics_resource_adaptor.hpp

@jrhemstad
Copy link
Collaborator

@ericniebler is this still something we are considering or can we close/update this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right. CUDA Next Feature intended for the Cuda Next experimental library
Projects
Status: Done
Development

No branches or pull requests

4 participants