Route zero-byte binning allocations through bin resources

[bdice]

Description

Routes zero-byte binning_memory_resource allocations through the selected bin resource instead of returning nullptr directly. Deallocations already route through the selected resource, so this keeps zero-byte allocation/deallocation behavior matched and preserves side effects from bin resources such as logging or tracking.

Adds a regression test that verifies zero-byte async and sync allocation/deallocation calls reach the bin resource.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR removes the zero-size short-circuit from binning_memory_resource_impl::allocate and allocate_sync, dispatching zero-byte requests to the bin resource. A new gMock test verifies zero-byte async/sync allocations return nullptr and that corresponding deallocations are invoked on the bin resource.

Changes

Zero-byte allocation behavior

Layer / File(s)	Summary
Allocate dispatch for zero bytes cpp/src/mr/detail/binning_memory_resource_impl.cpp	allocate and allocate_sync no longer short-circuit for bytes == 0; both call get_resource_ref(bytes).allocate(...) for zero-byte requests.
Zero-byte allocation/deallocation test cpp/tests/mr/binning_mr_tests.cpp	Adds gMock and mock resource header; new TEST(BinningTest, ZeroByteAllocationsUseBinResource) expects bin resource allocate(0) and deallocate(nullptr, 0) and asserts async/sync allocations return nullptr and deallocate paths are exercised.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• harrism

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title accurately reflects the main change: routing zero-byte allocations through bin resources instead of returning nullptr directly.
Description check	✅ Passed	The PR description accurately describes the changeset: it removes zero-byte early returns in allocations and adds a test verifying the behavior.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

cpp/tests/mr/binning_mr_tests.cpp (1)

31-43: ⚡ Quick win

Add alignment parameter for consistency.

Line 42 calls deallocate_sync with only 2 arguments (ptr and bytes), while line 41 explicitly passes the alignment parameter to deallocate. Based on the implementation signature at lines 87-89 of the implementation file, deallocate_sync requires 3 parameters: ptr, bytes, and alignment.

For consistency and clarity, consider adding the alignment parameter:

📝 Suggested change for consistency

   mr.deallocate(cuda_stream_view{}, nullptr, 0, rmm::CUDA_ALLOCATION_ALIGNMENT);
-  mr.deallocate_sync(nullptr, 0);
+  mr.deallocate_sync(nullptr, 0, rmm::CUDA_ALLOCATION_ALIGNMENT);

🤖 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/tests/mr/binning_mr_tests.cpp` around lines 31 - 43, The test calls
binning_mr::deallocate with an alignment but calls binning_mr::deallocate_sync
without it; update the test (BinningTest::ZeroByteDeallocateIsNoOp) to pass the
alignment to deallocate_sync as well (use the same
rmm::CUDA_ALLOCATION_ALIGNMENT constant) so the call matches the
deallocate_sync(ptr, bytes, alignment) signature in binning_mr and keeps
behavior consistent with deallocate.

🤖 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.

Nitpick comments:
In `@cpp/tests/mr/binning_mr_tests.cpp`:
- Around line 31-43: The test calls binning_mr::deallocate with an alignment but
calls binning_mr::deallocate_sync without it; update the test
(BinningTest::ZeroByteDeallocateIsNoOp) to pass the alignment to deallocate_sync
as well (use the same rmm::CUDA_ALLOCATION_ALIGNMENT constant) so the call
matches the deallocate_sync(ptr, bytes, alignment) signature in binning_mr and
keeps behavior consistent with deallocate.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: aa45d400-c99e-45f7-9942-f4922782d7cb

📥 Commits

Reviewing files that changed from the base of the PR and between b187020 and 15260ac.

📒 Files selected for processing (2)

• cpp/src/mr/detail/binning_memory_resource_impl.cpp
• cpp/tests/mr/binning_mr_tests.cpp

[bdice]

Actually the better fix here is to always route zero-byte allocations and deallocations through the corresponding resource. That way they can be tracked or logged if that bin's resource has side-effects. The bug is really just that allocation and deallocation don't match.

edit: Updated!

[bdice]

/merge