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

avoid cyclic ownership with GPU timers #1509

Merged
merged 2 commits into from
Dec 13, 2023
Merged

avoid cyclic ownership with GPU timers #1509

merged 2 commits into from
Dec 13, 2023

Conversation

upsj
Copy link
Member

@upsj upsj commented Dec 13, 2023

Discovered by @MarcelKoch, ProfilerHook::create(_nested)_summary doesn't work with GPU timers because they keep the executor alive via a cyclic dependency. This breaks up the dependency by storing device ID and stream explicitly.

Previously, attaching a ProfilerHook logger with a GPU timer lead to a dependency graph like this:

graph TD;
Executor --> ProfilerHook;
ProfilerHook --> CudaTimer;
CudaTimer -->|stream, device_id| Executor;
Loading

The new dependency graph removes the cycle

graph TD;
Executor --> ProfilerHook;
ProfilerHook --> CudaTimer;
CudaTimer --> stream;
CudaTimer --> device_id;
Loading

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label Dec 13, 2023
@upsj upsj requested review from MarcelKoch and a team December 13, 2023 15:43
@upsj upsj self-assigned this Dec 13, 2023
@ginkgo-bot ginkgo-bot added mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. mod:dpcpp This is related to the DPC++ module. labels Dec 13, 2023
include/ginkgo/core/base/timer.hpp Show resolved Hide resolved
include/ginkgo/core/base/timer.hpp Show resolved Hide resolved
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

Cool graphs! Didnt know GFM was capable of that. LGTM!

@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Dec 13, 2023
@upsj upsj merged commit 1e268ff into develop Dec 13, 2023
13 of 15 checks passed
@upsj upsj deleted the fix_gpu_timer branch December 13, 2023 20:08
Copy link

sonarcloud bot commented Dec 13, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

5 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:dpcpp This is related to the DPC++ module. mod:hip This is related to the HIP module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants