Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove MemorySpace::[de]allocate overloads without labels #6835

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

masterleinad
Copy link
Contributor

@masterleinad masterleinad commented Feb 21, 2024

I noticed in #6794 (comment) that we call MemorySpace::deallocate without a label only from internal allocations where we can do better.
Thus, this pull request suggests streamlining the interface by removing all overloads for MemorySpace::[de]allocate that don't take a label and attempts to include labels KokkosTools events in allocate and deallocate. So far, I only made sure to provide labels for SYCL in all places that need them now.
This also mirrors better what we do for View allocations anyway, i.e., we force the user to provide a label when allocating. The label is passed implicitly when deallocating whereas here it is explicit (since we also don't store it in SharedAllocationRecord or the like).

@masterleinad masterleinad force-pushed the remove_memory_space_allocate_without_labels branch from 4838e38 to 7965fc0 Compare February 21, 2024 22:39
@masterleinad masterleinad force-pushed the remove_memory_space_allocate_without_labels branch from 7965fc0 to ee43803 Compare February 22, 2024 16:16
@ldh4
Copy link
Contributor

ldh4 commented Mar 12, 2024

Should these [de]allocate functions go through a proper deprecation process? They are technically public functions in a non-impl namespace, available to be called in places external to kokkos.

@masterleinad
Copy link
Contributor Author

masterleinad commented Mar 12, 2024

Should these [de]allocate functions go through a proper deprecation process? They are technically public functions in a non-impl namespace, available to be called in places external to kokkos.

Yeah, that's up for debate. For me, they are implementation details but of course they could be used somewhere.

Copy link
Contributor

@ldh4 ldh4 left a comment

Choose a reason for hiding this comment

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

Other than the need for a discussion for a potential deprecation, looks good to me.

@masterleinad
Copy link
Contributor Author

Other than the need for a discussion for a potential deprecation, looks good to me.

Let me add a commit with the deprecations.

@masterleinad masterleinad force-pushed the remove_memory_space_allocate_without_labels branch from 3998a0c to 2cca334 Compare March 15, 2024 12:40
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Since when do we have these MemSpace::[de]allocate overload with a label vs the ones w/o a label?

What is problematic about having both?

Why is requiring a label the right thing to do?

core/src/Cuda/Kokkos_CudaSpace.cpp Show resolved Hide resolved
@masterleinad
Copy link
Contributor Author

Since when do we have these MemSpace::[de]allocate overload with a label vs the ones w/o a label?

The version with labels was introduced in #3084. The one without labels was the only overload before that.

What is problematic about having both?

There doesn't seem to be a good case where you cannot provide a meaningful label. We require a label for View allocations anyway. Not providing a label (possibly by accident, not knowing that the other overload existed) makes debugging [de]allocations events harder (provided the respective backend reports back to Tools events).

Why is requiring a label the right thing to do?

This pull request is motivated by #6794 (comment). It's very hard to understand Kokkos Tools output for allocation/deallocation events if the label is omitted. In that issue, we were trying to hunt down a specific deallocation that made a test introspecting Tools events fail (since there was an unexpected fence). With the changes in this pull request, we would have caught the offending deallocation directly.

@masterleinad masterleinad force-pushed the remove_memory_space_allocate_without_labels branch from 2cca334 to f7e0122 Compare April 4, 2024 12:19
@masterleinad masterleinad force-pushed the remove_memory_space_allocate_without_labels branch from 81efbd9 to 1714060 Compare April 4, 2024 12:42
@masterleinad masterleinad force-pushed the remove_memory_space_allocate_without_labels branch from f610030 to ea10a4b Compare April 4, 2024 22:02
Copy link
Contributor

@tcclevenger tcclevenger left a comment

Choose a reason for hiding this comment

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

I like the change.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

I am blocking for now. I am not sure I agree with removing the allocate function without label. I am even less sure I agree with removing the deallocate function with label - since it means that a user has to track the label around somewhere. And what would it mean to provide a different label at deallocate than was used on allocate.

@ajpowelsnl
Copy link
Contributor

@masterleinad , @crtrott -- on the agenda for discussion

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.

None yet

6 participants