Skip to content

Fix stream ownership and resource cleanup - #940

Merged
Changho Hwang (chhwang) merged 4 commits into
mainfrom
fix/stream-ownership-and-resource-cleanup-20261026
Nov 5, 2026
Merged

Changho Hwang (chhwang) merged 4 commits into
mainfrom
fix/stream-ownership-and-resource-cleanup-20261026

Conversation

@chhwang

Copy link
Copy Markdown
Contributor

Summary

  • Make CudaStreamWithFlags and GpuStream non-copyable to prevent duplicate stream destruction and returning a pool lease more than once. No move API is added; current direct-prvalue return usage remains valid under C++20 guaranteed copy elision. This intentionally breaks callers that copy either type, including std::move expressions that previously fell back to copying.
  • Free the successful verbs device list in getIBDeviceCount() and return zero if list acquisition returns null.
  • Initialize empty GpuBuffer state with zero bytes and deviceId() == -1, document that behavior, and extend the existing GPU-utils test.

Validation

  • Built the actual unit_tests and mp_unit_tests targets with CUDA 12.9, C++20, and IB enabled; ownership static_asserts compiled.
  • Existing GpuUtilsTest.StreamPool and GpuUtilsTest.AllocShared each passed with exactly one filtered test selected. The latter includes the empty-buffer assertions.
  • No full test suite or IB hardware runtime test was run.

This PR does not add locks, reference counting, synchronization, allocator changes, or benchmark/performance claims.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@chhwang
Changho Hwang (chhwang) marked this pull request as ready for review November 3, 2026 06:30
Copilot AI balanced review requested due to automatic review settings November 3, 2026 06:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues are reported for the stream-ownership, resource-cleanup, and empty-buffer changes.

No open issues 🎉

What changed in this PR

This PR tightens GPU stream ownership and resource cleanup in MSCCL++ utilities while defining predictable empty-buffer behavior.

Changes:

  • Makes both stream wrappers non-copyable to prevent duplicate destruction or pool returns; no move API is added.
  • Frees acquired InfiniBand device lists and returns zero when acquisition fails.
  • Initializes empty GPU buffers with zero bytes and device ID -1, documents the behavior, and adds regression assertions.
File Description
test/​unit/​gpu_utils_tests.cc Adds compile-time non-copyability checks and empty-buffer assertions.
src/​core/​ib.cc Releases the device list and handles null acquisition.
include/​mscclpp/​gpu_utils.hpp Deletes stream copy operations and initializes and documents empty-buffer state.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 3, 2026 23:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues are reported for the resource-ownership and cleanup changes.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 5, 2026 00:43
@chhwang
Changho Hwang (chhwang) merged commit d61b9a7 into main Nov 5, 2026
16 checks passed
@chhwang
Changho Hwang (chhwang) deleted the fix/stream-ownership-and-resource-cleanup-20261026 branch November 5, 2026 00:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

There are no active review issues for the ownership and resource-cleanup changes.

No open issues 🎉

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants