Skip to content

Ensure allocation stream is used for buffer deallocation if no explicit stream is provided - #1032

Merged
leofang merged 19 commits into
NVIDIA:mainfrom
leofang:dealloc_stream
Oct 7, 2025
Merged

Ensure allocation stream is used for buffer deallocation if no explicit stream is provided#1032
leofang merged 19 commits into
NVIDIA:mainfrom
leofang:dealloc_stream

Conversation

@leofang

Copy link
Copy Markdown
Member

Description

closes#779.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@copy-pr-bot

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@leofangleofang self-assigned this Sep 26, 2025
@leofangleofang added P0 High priority - Must do! cuda.core Everything related to the cuda.core module breaking Breaking changes are introduced labels Sep 26, 2025
@leofangleofang added this to the cuda.core beta 7 milestone Sep 26, 2025
@leofang

Copy link
Copy Markdown
MemberAuthor

/ok to test 738eed4

@github-actions

This comment has been minimized.

Comment threadcuda_core/cuda/core/experimental/_memory.pyx Outdated
Comment threadcuda_core/cuda/core/experimental/_memory.pyx Outdated
@leofangleofang added the blocked This task is currently blocked by other tasks label Oct 5, 2025
@leofang

Copy link
Copy Markdown
MemberAuthor

(This PR is now based entirely on #1070 except for the last commit, so that PR needs to be merged first.)

@leofang

Copy link
Copy Markdown
MemberAuthor

/ok to test 605241e

@leofang
leofang requested a review from shwinaOctober 6, 2025 13:13

@kkraus14kkraus14 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there any way we could add some testing for stream ordered allocations / deallocations?

Comment threadcuda_core/cuda/core/experimental/_memory.pyx Outdated
@kkraus14

Copy link
Copy Markdown
Collaborator

Changes LGTM on top of #1070

@leofangleofang removed the blocked This task is currently blocked by other tasks label Oct 7, 2025
@leofang
leofang marked this pull request as ready for review October 7, 2025 19:28
@leofang

Copy link
Copy Markdown
MemberAuthor

Merged with main!

@leofang

Copy link
Copy Markdown
MemberAuthor

/ok to test 33345d6

Comment threadcuda_core/cuda/core/experimental/_memory.pyx
If the buffer is deallocated without an explicit stream, the allocation stream
is used.
"""
if stream is None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did we intend to remove this? Based on the deallocate API signature a user would deem it valid to pass None as a stream argument. That None is not checked in the _deallocate function above when the stream object is called with it.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is a very interesting question that's worth expanding.. From my perspective mr.deallocate() is not meant to be called by end users because

  1. it was designed to be called by Buffer.close() or its destructor when out of scope
  2. it has such an ugly, not pythonic signature (passing pointer and size)

I don't think currently we have any test that actually exercise calling mr.deallocate() explicitly.

If this can be called by the end users, the contract that we deallocate using the allocation stream would not be upheld, because this API does not take any Buffer argument and so the stream is not available to us.

Comment threadcuda_core/cuda/core/experimental/_memory.pyx
@leofang
leofang merged commit 4cbb627 into NVIDIA:mainOct 7, 2025
71 checks passed
@leofang
leofang deleted the dealloc_stream branch October 7, 2025 22:33
@leofang

Copy link
Copy Markdown
MemberAuthor

Thanks, all! I noticed I forgot to prepare a release note for this PR. Working on it now...

@leofang

Copy link
Copy Markdown
MemberAuthor

Working on it now...

#1097

@github-actions

Copy link
Copy Markdown
Doc Preview CI
Preview removed because the pull request was closed or merged.

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

Labels

breakingBreaking changes are introducedcuda.coreEverything related to the cuda.core moduleP0High priority - Must do!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Buffer deallocation always happens on default stream

4 participants

@leofang@kkraus14@cpcloud@rparolin