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

Add asynchronous concurrent execution #3687

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Add asynchronous concurrent execution #3687

merged 1 commit into from
Jan 14, 2025

Conversation

matyas-streamhpc
Copy link

No description provided.

@matyas-streamhpc matyas-streamhpc self-assigned this Nov 25, 2024
@neon60 neon60 force-pushed the async-doc branch 2 times, most recently from 1484d67 to f81588d Compare December 2, 2024 08:46
@neon60 neon60 marked this pull request as ready for review December 2, 2024 08:53
@neon60 neon60 force-pushed the async-doc branch 4 times, most recently from fd5af51 to 6a139c6 Compare December 6, 2024 18:18
Copy link

@randyh62 randyh62 left a comment

Choose a reason for hiding this comment

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

left comments. Looks good overall.

docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
@neon60 neon60 force-pushed the async-doc branch 4 times, most recently from ed7e05f to 7cb3237 Compare December 19, 2024 17:42
@neon60 neon60 force-pushed the async-doc branch 2 times, most recently from 9592272 to e0ab3f3 Compare January 8, 2025 15:37
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Show resolved Hide resolved
@neon60 neon60 requested a review from cjatin January 10, 2025 11:44
Copy link

@AidanBeltonS AidanBeltonS left a comment

Choose a reason for hiding this comment

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

Overall LGTM, some minor comments

docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
Comment on lines 31 to 32
Streams enable the overlap of computation and data transfer, ensuring
continuous GPU activity.

Choose a reason for hiding this comment

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

[NIT] Seems vague and out of place in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to explain, why should we use streams added an extra sentence to avoid the "out of place"

Choose a reason for hiding this comment

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

The whole paragraph seems out of place. You come from section Streams and concurrent execution where you explain this idea. Then you restate it at the beginning of Managing streams. It is not wrong but it does not read well, and feels unnecessary/redundant. As it is a NIT, feel free to resolve this comment if you don't agree with the suggestion.

docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
docs/how-to/hip_runtime_api/asynchronous.rst Outdated Show resolved Hide resolved
Comment on lines 127 to 128
Asynchronous memory operations allow data to be transferred between the host
and device while kernels are being executed on the GPU. Using operations like
Copy link

@AidanBeltonS AidanBeltonS Jan 10, 2025

Choose a reason for hiding this comment

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

I think this sentence is a bit misleading. A reader could miss the fact that this operation must be on a different streams to get this behavior.

Asynchronous memory operations, do not block the host while copying this data.
Asynchronous memory operations on multiple streams allow for data to be transferred between the host and device while kernels are executed. (and do not block the host while copying this data)

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated sentence.

Comment on lines 102 to 104
One of the primary benefits of asynchronous operations is the ability to
overlap data transfer with kernel execution, leading to better resource
utilization and improved performance.

Choose a reason for hiding this comment

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

[NIT] you could clarify that multiple streams are needed to copy while executing a kernel in parallel.

another. This technique is especially useful in applications with large data
sets that need to be processed quickly.

Concurrent data transfers

Choose a reason for hiding this comment

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

How does this differ to the Asynchronous memory operations section?
This feels repetitive, and it is not clear on how you wish to distinguish between concurrent and asynchronous within this context

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed concurrent data transfers section and added hipMemcpyPeerAsync mention to previous section.

Copy link

@AidanBeltonS AidanBeltonS left a comment

Choose a reason for hiding this comment

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

Main comments were addressed

@neon60 neon60 merged commit befe0fe into docs/develop Jan 14, 2025
4 checks passed
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.

5 participants