NVIDIA / cccl

CUDA Core Compute Libraries
https://nvidia.github.io/cccl/
Other
1.18k stars 144 forks source link

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

Open ericniebler opened 4 weeks ago

ericniebler commented 4 weeks ago

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

jrhemstad commented 4 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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