[SYCL] Fix memory dependency leaks caused by failed kernel enqueue #5120

steffenlarsen · 2021-12-10T17:08:32Z

If a kernel enqueue fails the runtime will immediately try and clean it up. However, if it has any dependencies or users the cleanup will be skipped. This can cause the dependencies to stay alive and leak. These changes forces a full sub-graph cleanup of the command if enqueuing failed. Additionally, sub-graph cleanup is changed to account for failed kernel enqueues and will remove the failed command from its leaves.

If a kernel enqueue fails the runtime will immediately try and clean it up. However, if it has any dependencies or users the cleanup will be skipped. This can cause the dependencies to stay alive and leak. These changes forces a full sub-graph cleanup of the command if enqueuing failed. Additionally, sub-graph cleanup is changed to account for failed kernel enqueues and will remove the failed command from its leaves. Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

romanovvlad · 2021-12-10T17:35:19Z

@sergey-semenov Could you please take a look?

sycl/source/detail/scheduler/scheduler.cpp

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen · 2021-12-13T11:51:37Z

/summary:run

sycl/source/detail/scheduler/graph_builder.cpp

sycl/source/detail/scheduler/scheduler.cpp

steffenlarsen · 2021-12-14T08:28:20Z

I have converted this PR to a draft while I do further investigation into a more robust solution.

This commit adds a new strategy for cleaning up failed commands, namely to walk up the failed commands and its users and replace them by empty commands. This preserves the structure of the graph while replacing failed state. Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen · 2021-12-14T18:13:29Z

The new commit changes the strategy.

This commit adds a new strategy for cleaning up failed commands, namely
to walk up the failed commands and its users and replace them by empty
commands. This preserves the structure of the graph while replacing
failed state.

@romanovvlad | @dm-vodopyanov | @sergey-semenov - Thoughts on this strategy?

Additional things to consider:

Should the failed command and its users be merged into a single empty command? This command could have all the dependencies of the entire user-subgraph.
What happens when an auxiliary command fails? Is it enough to clean up the primary command and its users? Alternatively we could clean up the failed auxiliary command and all the ones there weren't enqueued after the first failure, then clean up the primary command.

Once a final solution has been found, I will document it in the relevant headers.

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

s-kanaev · 2022-01-12T14:09:09Z

Should the failed command and its users be merged into a single empty command? This command could have all the dependencies of the entire user-subgraph.

Sounds legit.

What happens when an auxiliary command fails? Is it enough to clean up the primary command and its users? Alternatively we could clean up the failed auxiliary command and all the ones there weren't enqueued after the first failure, then clean up the primary command.

I can't get what is an auxiliary command.

s-kanaev · 2022-01-12T14:11:18Z

Should there be any policy for back-off enqueue of failed command or it's user's responsibility to restore and re-enqueue required kernels/commands?

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen · 2022-02-11T18:02:03Z

Should the failed command and its users be merged into a single empty command? This command could have all the dependencies of the entire user-subgraph.

For simplicity, this is not implemented. I am not convinced it is the right approach to assume all users should be deleted to or if it is actually ever applicable.

What happens when an auxiliary command fails? Is it enough to clean up the primary command and its users? Alternatively >> we could clean up the failed auxiliary command and all the ones there weren't enqueued after the first failure, then clean up >> the primary command.

I can't get what is an auxiliary command.

addCG will first try to enqueue "auxiliary commands". These may cause the failure as well.

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen · 2022-02-14T14:35:37Z

@sergey-semenov @dm-vodopyanov - I believe this is ready for review.

steffenlarsen · 2022-03-01T19:38:11Z

@dm-vodopyanov & @sergey-semenov - Friendly ping.

sergey-semenov

Sorry for the late review, the changes LGTM.

sergey-semenov · 2022-03-02T12:38:47Z

sycl/source/detail/scheduler/graph_builder.cpp

+  if (FailedCmd->getType() == Command::CommandType::RUN_CG) {
+    auto ExecCmd = static_cast<ExecCGCommand *>(FailedCmd);
+    std::vector<std::shared_ptr<stream_impl>> Streams = ExecCmd->getStreams();
+    ExecCmd->clearStreams();
+    StreamsToDeallocate.insert(StreamsToDeallocate.end(), Streams.begin(),
+                               Streams.end());


Should we do similar handling for reduction resources here once #5653 lands?

Yes, absolutely! I will make sure it is included into whichever is merged last.

…queue (#5120)" This reverts commit 104e439.

…queue (#5120)" (#5718) This reverts commit 104e439. #5120 is causing post-commit failures and performance regressions. This reverts it.

steffenlarsen · 2022-03-03T10:16:32Z

Due to performance regressions and test failures in post-commit, these changes have been reverted. I will revisit this shortly.

…ntel#5120) If a kernel enqueue fails the runtime will immediately try and clean it up. However, if it has any dependencies or users the cleanup will be skipped. This can cause the dependencies to stay alive and leak. These changes forces a full sub-graph cleanup of the command if enqueuing failed. Additionally, sub-graph cleanup is changed to account for failed kernel enqueues and will remove the failed command from its leaves.

steffenlarsen requested review from dm-vodopyanov and romanovvlad December 10, 2021 17:08

steffenlarsen requested a review from a team as a code owner December 10, 2021 17:08

dm-vodopyanov reviewed Dec 10, 2021

View reviewed changes

sycl/source/detail/scheduler/scheduler.cpp Outdated Show resolved Hide resolved

steffenlarsen requested a review from sergey-semenov December 13, 2021 08:34

steffenlarsen added 2 commits December 13, 2021 11:46

Fix format and tests

ae4f2a8

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Correct conditional

050b4f2

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

sergey-semenov reviewed Dec 13, 2021

View reviewed changes

sycl/source/detail/scheduler/graph_builder.cpp Outdated Show resolved Hide resolved

sycl/source/detail/scheduler/scheduler.cpp Outdated Show resolved Hide resolved

steffenlarsen marked this pull request as draft December 14, 2021 08:27

steffenlarsen changed the title ~~[SYCL] Fixes memory dependency leaks caused by failed kernel enqueue~~ Draft: [SYCL] Fixes memory dependency leaks caused by failed kernel enqueue Dec 14, 2021

steffenlarsen added 3 commits December 14, 2021 21:14

Fix formatting

eafd2e6

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Add fast-path for dependencyless and userless commands

5811dc9

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Merge failed command and users into a single empty command

2908a13

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

s-kanaev requested a review from dm-vodopyanov January 10, 2022 10:27

steffenlarsen added 4 commits February 3, 2022 16:52

Merge 'intel/sycl' into steffen/fix_enqueue_fail_leak

8cddbd2

Move cleanup

35ba203

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Ensure adoption of dependencies

88a1404

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Simplify approach and extend replacement operations

2ecb99a

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Remove unused function

5daf07a

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen marked this pull request as ready for review February 14, 2022 14:34

steffenlarsen changed the title ~~Draft: [SYCL] Fixes memory dependency leaks caused by failed kernel enqueue~~ [SYCL] Fixes memory dependency leaks caused by failed kernel enqueue Feb 15, 2022

bader requested a review from sergey-semenov February 22, 2022 09:59

bader changed the title ~~[SYCL] Fixes memory dependency leaks caused by failed kernel enqueue~~ [SYCL] Fix memory dependency leaks caused by failed kernel enqueue Feb 22, 2022

sergey-semenov approved these changes Mar 2, 2022

View reviewed changes

bader merged commit 104e439 into intel:sycl Mar 2, 2022

steffenlarsen added a commit that referenced this pull request Mar 3, 2022

Revert "[SYCL] Fix memory dependency leaks caused by failed kernel en…

5226c2e

…queue (#5120)" This reverts commit 104e439.

steffenlarsen mentioned this pull request Mar 3, 2022

Revert "[SYCL] Fix memory dependency leaks caused by failed kernel enqueue" #5718

Merged

bader pushed a commit that referenced this pull request Mar 3, 2022

Revert "[SYCL] Fix memory dependency leaks caused by failed kernel en…

bb0261c

…queue (#5120)" (#5718) This reverts commit 104e439. #5120 is causing post-commit failures and performance regressions. This reverts it.

bader mentioned this pull request Mar 5, 2022

[SYCL] Fix memory leak in reduction resources #5653

Merged

steffenlarsen mentioned this pull request Mar 11, 2022

Draft: [SYCL] Fixes memory dependency leaks caused by failed kernel enqueue #5788

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Fix memory dependency leaks caused by failed kernel enqueue #5120

[SYCL] Fix memory dependency leaks caused by failed kernel enqueue #5120

Uh oh!

steffenlarsen commented Dec 10, 2021

Uh oh!

romanovvlad commented Dec 10, 2021

Uh oh!

Uh oh!

steffenlarsen commented Dec 13, 2021

Uh oh!

Uh oh!

Uh oh!

steffenlarsen commented Dec 14, 2021

Uh oh!

steffenlarsen commented Dec 14, 2021 •

edited

Loading

Uh oh!

s-kanaev commented Jan 12, 2022

Uh oh!

s-kanaev commented Jan 12, 2022

Uh oh!

steffenlarsen commented Feb 11, 2022

Uh oh!

steffenlarsen commented Feb 14, 2022

Uh oh!

steffenlarsen commented Mar 1, 2022

Uh oh!

sergey-semenov left a comment

Uh oh!

sergey-semenov Mar 2, 2022

Uh oh!

steffenlarsen Mar 2, 2022

Uh oh!

steffenlarsen commented Mar 3, 2022

Uh oh!

Uh oh!

[SYCL] Fix memory dependency leaks caused by failed kernel enqueue #5120

[SYCL] Fix memory dependency leaks caused by failed kernel enqueue #5120

Uh oh!

Conversation

steffenlarsen commented Dec 10, 2021

Uh oh!

romanovvlad commented Dec 10, 2021

Uh oh!

Uh oh!

steffenlarsen commented Dec 13, 2021

Uh oh!

Uh oh!

Uh oh!

steffenlarsen commented Dec 14, 2021

Uh oh!

steffenlarsen commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-kanaev commented Jan 12, 2022

Uh oh!

s-kanaev commented Jan 12, 2022

Uh oh!

steffenlarsen commented Feb 11, 2022

Uh oh!

steffenlarsen commented Feb 14, 2022

Uh oh!

steffenlarsen commented Mar 1, 2022

Uh oh!

sergey-semenov left a comment

Choose a reason for hiding this comment

Uh oh!

sergey-semenov Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Mar 3, 2022

Uh oh!

Uh oh!

steffenlarsen commented Dec 14, 2021 •

edited

Loading