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

[FEA]: provide a type erased owning resource wrapper #1426

Closed
1 task done
miscco opened this issue Feb 22, 2024 · 16 comments · Fixed by #2266
Closed
1 task done

[FEA]: provide a type erased owning resource wrapper #1426

miscco opened this issue Feb 22, 2024 · 16 comments · Fixed by #2266
Assignees
Labels
feature request New feature or request.

Comments

@miscco
Copy link
Collaborator

miscco commented Feb 22, 2024

Is this a duplicate?

Area

libcu++

Is your feature request related to a problem? Please describe.

libcu++ currently provides the {async_}resource_ref wrapper that provides type erased access to an existing memory resource.

However, often we want to also store a type erased memory resource to then pass around through a {async_}resource_ref

Describe the solution you'd like

Following convention, I would propose to name it any_resource. Obvious use cases are testing frameworks which usually test APIs against a host of different resources and also certain resource adaptors that want to adopt a memory resource.

Finally this would also allow it to create something similar to rmm:{get,set}_current_device_resource.

Just throwing ideas around I believe it would be sufficient to only store few methods in an external vtable

  • destroy we need to redirect to the correct destructor during destruction of any_resource
  • to_resource_ref we need to have a static method that creates a {async_}resource_ref with the right member functions. We can use this through operator-> or operator*

Describe alternatives you've considered

Just using a std::any does not work, because we need a good way to actually create a {async_}resource_ref from the stored type erased resource.

Additional context

No response

@miscco miscco added the feature request New feature or request. label Feb 22, 2024
@github-project-automation github-project-automation bot moved this to Todo in CCCL Feb 22, 2024
@miscco miscco self-assigned this Feb 22, 2024
@miscco
Copy link
Collaborator Author

miscco commented Feb 22, 2024

@harrism Would love to get your input here

@jrhemstad
Copy link
Collaborator

Can you provide a sketch of what using any_resource would look like?

@miscco
Copy link
Collaborator Author

miscco commented Feb 22, 2024

Regarding the mentioned two use cases

A way to store the memory resource used per device (shamelessly stolen from rmm)

using namespace cuda::mr;

inline auto& get_map()
{
  static std::map<cuda_device_id::value_type, any_resource_with<device_accessible>> device_id_to_resource;
  return device_id_to_resource;
}
async_resource_ref<device_accessible> get_per_device_resource(cuda_device_id device_id)
{
  std::lock_guard<std::mutex> lock{detail::map_lock()};
  auto& map = detail::get_map();
  // If a resource was never set for `id`, set to the initial resource
  auto const found = map.find(device_id.value());
  return (found == map.end()) ? (map[device_id.value()] = detail::initial_resource())
                              :  found->second;
}

A test framework for different memory resources again stolen from rmm

using MRFactoryFunc = std::function<std::shared_ptr<any_resource_with<device_accessible>>()>;

A container that actually stores a memory resource

class my_container {

private:
  any_resource_with<...> mr_;
  void* __ptr;
  ....
};

@miscco
Copy link
Collaborator Author

miscco commented Feb 22, 2024

In general any RAII type memory functionality kind of wants to actually store the used memory resource and not rely on get_current_device_resource because that could have actually changed since it was constructed

@jrhemstad
Copy link
Collaborator

In general any RAII type memory functionality kind of wants to actually store the used memory resource

Specifically, you mean storing an owning reference to the resource? Because otherwise, you could simply store a resource_ref.

@miscco
Copy link
Collaborator Author

miscco commented Feb 22, 2024

Yeah this is explicitly about owning a resource

@jrhemstad
Copy link
Collaborator

jrhemstad commented Feb 22, 2024

Would any_resource be a shared ownership model? Or exclusive ownership?

What would it look like to construct an any_resource from an instance of an arbitrary type R that satisfies the resource<R> concept?

@harrism
Copy link
Contributor

harrism commented Feb 26, 2024

Following convention, I would propose to name it any_resource.

I'm not familiar with this convention. Can you provide references?

Why don't C++ smart pointers (std::unique_pointer) work for this?

@harrism
Copy link
Contributor

harrism commented Feb 26, 2024

Final question: would it be better to try this out in RMM first to work out the kinks before canonizing it in CCCL? After all the use cases are in RMM...

@miscco
Copy link
Collaborator Author

miscco commented Feb 26, 2024

The convention is that std::any is a type erased thing and resource is what we want, so combining them would yield any_resource

@miscco
Copy link
Collaborator Author

miscco commented Feb 26, 2024

Regarding the question about unique_ptr, that should be doable, but what we also want is something that lives on the stack and not the heap

@harrism
Copy link
Contributor

harrism commented Feb 26, 2024

Regarding the question about unique_ptr, that should be doable, but what we also want is something that lives on the stack and not the heap

Can you add this point (and a reason why) to the issue description?

@jrhemstad
Copy link
Collaborator

I'm not familiar with this convention. Can you provide references?

std::any is a standard polymorphic abstraction that can be "any" type. It's an owning object.

resource_ref is a reference to an object that is a resource.

any_resource is an instance of an object that is a resource.

Why don't C++ smart pointers (std::unique_pointer) work for this?

unique_ptr is a pointer to an instance of a heap allocated object. any_resource would be an instance of the object.

@harrism
Copy link
Contributor

harrism commented Feb 27, 2024

Yep, I think that's what Michael said already. :) Just asking for it to be all documented in the issue description.

@harrism
Copy link
Contributor

harrism commented Apr 23, 2024

We will need this implemented in order to move forward with removing RMM's MR base classes.

@harrism
Copy link
Contributor

harrism commented May 7, 2024

Couple more questions.

  1. I'd like to see the answers to these questions from @jrhemstad above:

Would any_resource be a shared ownership model? Or exclusive ownership?

What would it look like to construct an any_resource from an instance of an arbitrary type R that satisfies the resource<R> concept?

  1. I recall that any_resource_ref was the original planned name for what is now resource_ref. Why was any dropped from that?

  2. The naming is a bit confusing:

  • resource_ref: non-owning type-erased wrapper with specified template properties
  • any_resource_with: owning type-erased wrapper with specified template properties
  • resource_with: a concept with specified template properties

So:

  • ref == non-owning, type-erased
  • with == specified template properties

To me, it seems like for consistency resource_ref should be resource_ref_with, or any_resource should not have _with.

@jrhemstad jrhemstad assigned ericniebler and unassigned miscco Aug 14, 2024
miscco added a commit to miscco/cccl that referenced this issue Aug 15, 2024
ericniebler pushed a commit to ericniebler/cccl that referenced this issue Aug 20, 2024
@cccl-authenticator-app cccl-authenticator-app bot moved this from Todo to In Review in CCCL Aug 20, 2024
@cccl-authenticator-app cccl-authenticator-app bot moved this from In Review to In Progress in CCCL Aug 20, 2024
@cccl-authenticator-app cccl-authenticator-app bot moved this from In Progress to In Review in CCCL Aug 22, 2024
@cccl-authenticator-app cccl-authenticator-app bot moved this from In Review to In Progress in CCCL Aug 23, 2024
miscco added a commit that referenced this issue Aug 27, 2024
…2266)

* Implement `any_resource` an owning wrapper around any resource

Addresses #1426

* Continue development of @miscco's `any_resource`

* address review feedback

* [pre-commit.ci] auto code formatting

* mark all deallocation functions as `noexcept`

* fix some test failures

* more tests and bug fixes

* fix more build breaks

* attempt to fix the cudax docs build

* exclude more symbols from the cudax docs

* more portability fixes and doxygen tweaks

* once more with feeling

* getting pretty close now

* fix broken test

* deduplicate `basic_any_resource` constructors to satisfy doxygen

* [pre-commit.ci] auto code formatting

* don't use `if constexpr` when compiling as c++14

* more fixes for doxygen and c++14

* back out a questionable addition of `noexcept`

* molify msvc

* accommodate integer size differences on msvc

* eliminate shadow warning treated as error

* handle duplicate properties without triggering compiler warnings

* [pre-commit.ci] auto code formatting

---------

Co-authored-by: Michael Schellenberger Costa <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Done in CCCL Aug 27, 2024
pciolkosz pushed a commit to pciolkosz/cccl that referenced this issue Aug 27, 2024
…VIDIA#2266)

* Implement `any_resource` an owning wrapper around any resource

Addresses NVIDIA#1426

* Continue development of @miscco's `any_resource`

* address review feedback

* [pre-commit.ci] auto code formatting

* mark all deallocation functions as `noexcept`

* fix some test failures

* more tests and bug fixes

* fix more build breaks

* attempt to fix the cudax docs build

* exclude more symbols from the cudax docs

* more portability fixes and doxygen tweaks

* once more with feeling

* getting pretty close now

* fix broken test

* deduplicate `basic_any_resource` constructors to satisfy doxygen

* [pre-commit.ci] auto code formatting

* don't use `if constexpr` when compiling as c++14

* more fixes for doxygen and c++14

* back out a questionable addition of `noexcept`

* molify msvc

* accommodate integer size differences on msvc

* eliminate shadow warning treated as error

* handle duplicate properties without triggering compiler warnings

* [pre-commit.ci] auto code formatting

---------

Co-authored-by: Michael Schellenberger Costa <[email protected]>
miscco added a commit that referenced this issue Aug 28, 2024
…`cudax::any_resource` (#2293)

* Implement `any_resource` an owning wrapper around any resource

Addresses #1426

* Continue development of @miscco's `any_resource`

* address review feedback

* [pre-commit.ci] auto code formatting

* mark all deallocation functions as `noexcept`

* fix some test failures

* more tests and bug fixes

* fix more build breaks

* attempt to fix the cudax docs build

* exclude more symbols from the cudax docs

* more portability fixes and doxygen tweaks

* once more with feeling

* getting pretty close now

* fix broken test

* deduplicate `basic_any_resource` constructors to satisfy doxygen

* [pre-commit.ci] auto code formatting

* don't use `if constexpr` when compiling as c++14

* more fixes for doxygen and c++14

* back out a questionable addition of `noexcept`

* molify msvc

* accommodate integer size differences on msvc

* eliminate shadow warning treated as error

* handle duplicate properties without triggering compiler warnings

* change `uninitialized_buffer` to own its memory resource using `any_resource`

* Use fully qualified name

* Drop `__host__ __device__` from uninitialized_buffer

* Revert "Drop `__host__ __device__` from uninitialized_buffer"

This reverts commit 5115b08.

* Just do the cursed thing

* Add missing include

* Adopt the doc string

---------

Co-authored-by: Michael Schellenberger Costa <[email protected]>
Co-authored-by: anon <users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
bernhardmgruber pushed a commit to bernhardmgruber/cccl that referenced this issue Aug 28, 2024
…VIDIA#2266)

* Implement `any_resource` an owning wrapper around any resource

Addresses NVIDIA#1426

* Continue development of @miscco's `any_resource`

* address review feedback

* [pre-commit.ci] auto code formatting

* mark all deallocation functions as `noexcept`

* fix some test failures

* more tests and bug fixes

* fix more build breaks

* attempt to fix the cudax docs build

* exclude more symbols from the cudax docs

* more portability fixes and doxygen tweaks

* once more with feeling

* getting pretty close now

* fix broken test

* deduplicate `basic_any_resource` constructors to satisfy doxygen

* [pre-commit.ci] auto code formatting

* don't use `if constexpr` when compiling as c++14

* more fixes for doxygen and c++14

* back out a questionable addition of `noexcept`

* molify msvc

* accommodate integer size differences on msvc

* eliminate shadow warning treated as error

* handle duplicate properties without triggering compiler warnings

* [pre-commit.ci] auto code formatting

---------

Co-authored-by: Michael Schellenberger Costa <[email protected]>
miscco added a commit to miscco/cccl that referenced this issue Aug 28, 2024
…`cudax::any_resource` (NVIDIA#2293)

* Implement `any_resource` an owning wrapper around any resource

Addresses NVIDIA#1426

* Continue development of @miscco's `any_resource`

* address review feedback

* [pre-commit.ci] auto code formatting

* mark all deallocation functions as `noexcept`

* fix some test failures

* more tests and bug fixes

* fix more build breaks

* attempt to fix the cudax docs build

* exclude more symbols from the cudax docs

* more portability fixes and doxygen tweaks

* once more with feeling

* getting pretty close now

* fix broken test

* deduplicate `basic_any_resource` constructors to satisfy doxygen

* [pre-commit.ci] auto code formatting

* don't use `if constexpr` when compiling as c++14

* more fixes for doxygen and c++14

* back out a questionable addition of `noexcept`

* molify msvc

* accommodate integer size differences on msvc

* eliminate shadow warning treated as error

* handle duplicate properties without triggering compiler warnings

* change `uninitialized_buffer` to own its memory resource using `any_resource`

* Use fully qualified name

* Drop `__host__ __device__` from uninitialized_buffer

* Revert "Drop `__host__ __device__` from uninitialized_buffer"

This reverts commit 5115b08.

* Just do the cursed thing

* Add missing include

* Adopt the doc string

---------

Co-authored-by: Michael Schellenberger Costa <[email protected]>
Co-authored-by: anon <users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants