Restore resource_cast memory limit check#1231
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates the CUDA helpers header to include RMM/RAFT memory-resource headers and changes get_device_memory_size() to consult the current RMM device resource, cast to a limiting_resource_adaptor if available, and return min(cuda-reported total_mem, adaptor allocation_limit); otherwise it returns total_mem. ChangesDevice memory reporting with RMM limiting adaptors
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/utilities/cuda_helpers.cuh (1)
254-259: ⚡ Quick winGate debug prints behind a debug flag.
Unconditional
printfinget_device_memory_size()can spam stdout and add sync overhead when this helper is called frequently. Consider guarding these lines with a compile-time debug flag.Suggested patch
- printf("limiting_adaptor->get_allocation_limit(): %fMiB\n", - limiting_adaptor->get_allocation_limit() / (double)1e6); - printf("used_mem: %fMiB\n", limiting_adaptor->get_allocated_bytes() / (double)1e6); - printf("free_mem: %fMiB\n", - (limiting_adaptor->get_allocation_limit() - limiting_adaptor->get_allocated_bytes()) / - (double)1e6); +#ifdef CUOPT_ENABLE_RESOURCE_LIMIT_DEBUG + printf("limiting_adaptor->get_allocation_limit(): %fMiB\n", + limiting_adaptor->get_allocation_limit() / (double)1e6); + printf("used_mem: %fMiB\n", limiting_adaptor->get_allocated_bytes() / (double)1e6); + printf("free_mem: %fMiB\n", + (limiting_adaptor->get_allocation_limit() - limiting_adaptor->get_allocated_bytes()) / + (double)1e6); +#endif🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/utilities/cuda_helpers.cuh` around lines 254 - 259, The three unconditional printf calls inside get_device_memory_size() (printing limiting_adaptor->get_allocation_limit(), get_allocated_bytes(), and free memory) should be wrapped in a compile-time debug macro so they don't run in production; add a macro guard (e.g., CUDA_HELPERS_DEBUG or similar) around those prints in cpp/src/utilities/cuda_helpers.cuh and only emit the diagnostics when the macro is defined, keeping the rest of get_device_memory_size() unchanged. Ensure the macro name is documented in the header comment so callers can enable it at build time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/utilities/cuda_helpers.cuh`:
- Around line 254-259: The printed memory diagnostics use the label "MiB" but
divide by 1e6 (decimal MB); update the three printf calls that reference
limiting_adaptor->get_allocation_limit() and
limiting_adaptor->get_allocated_bytes() to divide by 1024.0 * 1024.0 (or use a
named constant like BYTES_PER_MIB) so the units correctly reflect mebibytes, and
keep the "MiB" labels unchanged.
---
Nitpick comments:
In `@cpp/src/utilities/cuda_helpers.cuh`:
- Around line 254-259: The three unconditional printf calls inside
get_device_memory_size() (printing limiting_adaptor->get_allocation_limit(),
get_allocated_bytes(), and free memory) should be wrapped in a compile-time
debug macro so they don't run in production; add a macro guard (e.g.,
CUDA_HELPERS_DEBUG or similar) around those prints in
cpp/src/utilities/cuda_helpers.cuh and only emit the diagnostics when the macro
is defined, keeping the rest of get_device_memory_size() unchanged. Ensure the
macro name is documented in the header comment so callers can enable it at build
time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 430867ae-8a10-47b2-948f-2993d59e032c
📒 Files selected for processing (1)
cpp/src/utilities/cuda_helpers.cuh
bdice
added
non-breaking
|
Requesting @rg20 's review |
| auto res = rmm::mr::get_current_device_resource_ref(); | ||
| auto limiting_adaptor = cuda::mr::resource_cast<rmm::mr::limiting_resource_adaptor>(&res); | ||
| if (limiting_adaptor) { | ||
| printf("limiting_adaptor->get_allocation_limit(): %fMiB\n", |
There was a problem hiding this comment.
This is just restoring previous code. I can remove it, just noting that I didn't originally add it.
|
Actionable comments posted: 0 |
|
🔔 Hi @anandhkb @bdice, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
3 participants
Description
Restore the memory-limit-aware branch in
get_device_memory_size()that was temporarily removed during the CCCL/RMM memory-resource migration in #1035.The function now uses
cuda::mr::resource_castonrmm::mr::get_current_device_resource_ref()to detect an activermm::mr::limiting_resource_adaptor. When present, it reports the limit/usage/free values and returns the smaller of CUDA total memory and the adaptor allocation limit, matching the previous behavior.