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

[3/N] Add critical path analysis GPU->GPU sync dependencies #69

Closed
wants to merge 3 commits into from

Conversation

briancoutinho
Copy link
Contributor

@briancoutinho briancoutinho commented Oct 9, 2023

What does this PR do?

This final PR adds the smarts to infer inter stream (GPU->GPU) synchronization dependencies.

How it works?

The core part of the logic is to understand synchronization due to CUDA events.

  1. We look at all cudaEventRecord() calls on the CPU and identify the kernel launch preceeding it on a thread (see _get_cuda_event_record_df()
  2. CUPTI provides WaitEventSync events that include metadata "wait_on_cuda_event_record_corr_id". This the correlation ID of the cudaEventRecord() call that the synchronization takes place on.
  3. We can lookup the cudaEvent record id from step 2 using the dataframe from step 1. Now, we add a dependency between the previous launch before the cudaEventRecord and the next kernel after the WaitEventSync.

(Happy to discuss in person - this was convoluted)

Other changes

  1. Factored out the type of events considered as runtime CUDA launches into get_runtime_launch_events_query(). This is used in kernel analyzer as well.
  2. Added alexnet input test data too since it has GPU->GPU sync example

Test:
Below is an example of inter GPU stream sync, see arrow between events.
Screenshot 2023-10-09 at 10 34 14 AM

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
    • N/A
  • Did you write any new necessary tests?
    • N/A
  • Did you make sure to update the docs?
    • N/A Docs are coming in a follow up PR.
  • Did you update the changelog?
    • N/A

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 9, 2023
@facebook-github-bot
Copy link
Contributor

@briancoutinho has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@briancoutinho briancoutinho marked this pull request as ready for review October 9, 2023 18:56
Copy link
Contributor

@anupambhatnagar anupambhatnagar left a comment

Choose a reason for hiding this comment

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

🚢

.sort_values(by="ts", axis=0)
)

def previous_launch(ts: int, pid: int, tid: int) -> Optional[int]:
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to _previous_launch?

def previous_launch(ts: int, pid: int, tid: int) -> Optional[int]:
"""Find the previous CUDA launch on same pid and tid"""
df = runtime_calls.query(f"pid == {pid} and tid == {tid}")
lowerneighbours = df[df["ts"] < ts]["ts"]
Copy link
Contributor

Choose a reason for hiding this comment

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

*nit - lower_neighbors

@codecov-commenter
Copy link

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (7780150) 90.94% compared to head (3f71253) 91.06%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
+ Coverage   90.94%   91.06%   +0.11%     
==========================================
  Files          30       30              
  Lines        2507     2562      +55     
==========================================
+ Hits         2280     2333      +53     
- Misses        227      229       +2     
Files Coverage Δ
hta/analyzers/trace_counters.py 96.38% <ø> (-0.21%) ⬇️
hta/common/trace.py 88.75% <100.00%> (+0.20%) ⬆️
tests/test_trace_analysis.py 99.60% <100.00%> (+0.02%) ⬆️
hta/analyzers/critical_path_analysis.py 91.19% <95.34%> (+0.62%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@facebook-github-bot
Copy link
Contributor

@briancoutinho has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@briancoutinho merged this pull request in 66e3c54.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants