Skip to content

Ensure that our memory pools can implicitly convert to a resource_ref#7390

Merged
miscco merged 3 commits intoNVIDIA:mainfrom
miscco:convert_memory_pool
Feb 18, 2026
Merged

Ensure that our memory pools can implicitly convert to a resource_ref#7390
miscco merged 3 commits intoNVIDIA:mainfrom
miscco:convert_memory_pool

Conversation

@miscco
Copy link
Copy Markdown
Contributor

@miscco miscco commented Jan 28, 2026

We want to pass them to APIs that take only a resource_ref.

However, because those are not copyable we would need to convert them to a memory_pool_ref first.

However, that also does not work, because the constructor for resource_ref does not accept a prvalue.

To alleviate that create a conversion operator that does the right thing.

@miscco miscco requested a review from a team as a code owner January 28, 2026 09:50
@miscco miscco requested a review from griwes January 28, 2026 09:50
@github-project-automation github-project-automation bot moved this to Todo in CCCL Jan 28, 2026
@cccl-authenticator-app cccl-authenticator-app bot moved this from Todo to In Review in CCCL Jan 28, 2026
@miscco miscco force-pushed the convert_memory_pool branch 3 times, most recently from c155f43 to 5b34a2a Compare January 28, 2026 09:58
@github-actions

This comment has been minimized.

@pciolkosz
Copy link
Copy Markdown
Contributor

However, that also does not work, because the constructor for resource_ref does not accept a prvalue.

Would it make sense to return a X_memory_pool_ref& from as_ref() to change this? It most likely has its own set of issues as any function returning references. But the proposed change has a bit of a surprising effect for this sequence:

device_memory_pool pool{dev0};
resource_ref ref = pool;
any_resource res = ref;

We support turning resource_ref into any_resource by creating a copy of the resource. Because pool -> resource_ref conversion also downgraded the resource from owning to non-owning, when you create any_resource you end up having only the non-owning device_memory_pool_ref wrapped inside. I think I would prefer there to be an explicit as_ref() call somewhere in this sequence, so the user explicitly opts-in into this ownership change of the resource. It's not a strong concern though and if we think this is still the best way to make resource_ref more compatible with the memory pools I would be fine with the proposed changes

@pciolkosz
Copy link
Copy Markdown
Contributor

pciolkosz commented Feb 3, 2026

@miscco We discussed this on the working group meeting today and a suggestion from @ericniebler was to move the conversion operator from X_memory_pool to X_memory_pool_ref, so that we can keep the conversion to X_memory_pool_ref explicit with .as_ref(), but it should now work as an argument to a function expecting a resource_ref when user call .as_ref() on it. Basically solves the prvalue problem from the PR description

@miscco
Copy link
Copy Markdown
Contributor Author

miscco commented Feb 17, 2026

@pciolkosz I tried your approach but that is not possible without an intermediary.

The issue is that the resource_ref stores a pointer to the temporary created within as_ref so this will just dangle

@miscco miscco force-pushed the convert_memory_pool branch 2 times, most recently from eeb2161 to 241bc9b Compare February 17, 2026 09:02
@miscco
Copy link
Copy Markdown
Contributor Author

miscco commented Feb 17, 2026

@pciolkosz I believe I found an elegant solution to the issue at hand.

The underlying problem is that the construction to a meow_pool_ref creates a temporary.

However, a relatively simple solution is to just downcast to the meow_pool_ref in as_ref() and return by reference

There is no functional difference to the current constructor call, because that is what we actually store and the reference allows us to directly convert into a resource_ref

I believe that gives us the best of both worlds

@miscco miscco force-pushed the convert_memory_pool branch 2 times, most recently from d111cdd to ecf7660 Compare February 17, 2026 09:06
@github-actions

This comment has been minimized.

@miscco miscco force-pushed the convert_memory_pool branch from ecf7660 to 7e01f32 Compare February 17, 2026 18:02
We want to pass them to APIs that take only a `resource_ref`.

However, because those are not copyable we would need to convert them to a `memory_pool_ref` first.

However, that also does not work, because the constructor for `resource_ref` does not accept a prvalue.

To alleviate that create a conversion operator that does the right thing.
@miscco miscco force-pushed the convert_memory_pool branch from 7e01f32 to 0c5ed59 Compare February 17, 2026 18:03
@github-actions

This comment has been minimized.

Comment thread libcudacxx/test/libcudacxx/cuda/memory_resource/resources/memory_pools.cu Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@miscco miscco enabled auto-merge (squash) February 18, 2026 19:19
@github-actions
Copy link
Copy Markdown
Contributor

🥳 CI Workflow Results

🟩 Finished in 1h 21m: Pass: 100%/99 | Total: 1d 02h | Max: 55m 29s | Hits: 97%/251185

See results here.

@miscco miscco merged commit bde4700 into NVIDIA:main Feb 18, 2026
113 of 116 checks passed
@miscco miscco deleted the convert_memory_pool branch February 18, 2026 19:49
@github-project-automation github-project-automation bot moved this from In Review to Done in CCCL Feb 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants