[SYCL] Clear event_impl dependencies with graph cleanup #4793

maximdimakov · 2021-10-21T10:02:28Z

When event_impl's d-tor is called its dependencies are starting to recursive releasing. It leads to stack overflow.
Clearing event_impl dependencies in graph cleanup helps to eliminate this problem.
getWaitList was moved to event_impl class so the work with dependencies could be wrapped to mutex.
Test : intel/llvm-test-suite#574
Signed-off-by: mdimakov [email protected]

Signed-off-by: mdimakov <[email protected]>

maximdimakov · 2021-10-21T10:31:11Z

/summary:run

s-kanaev

Looks good.
The main concern of this solution is need to store all elements of graph (events) in a container.

There might be another solution with DFS employed instead of BFS.
Within this approach, The deepest event is to be released. A container is used for storing path to current graph element. Prior to moving up in a graph, vectors of dependencies of current event should be cleared.
Using raw pointers in path storage container will also reduce amount of calls to copy c-tor of shared ptr which will neglect the performance impact.

s-kanaev · 2021-10-21T10:51:52Z

sycl/source/detail/event_impl.cpp

+    // When d-tor of any event_impl except for Head is called
+    // the vectors of dependencies must be clean
+    for (auto &DepPtr : MPreparedDepsEvents) {
+      assert(DepPtr && "Dependencies list is not clean");


The assert message doesn't make sense here.
It should be smth like "Only valid dependencies are expected" etc.

s-kanaev · 2021-10-25T11:25:21Z

sycl/source/detail/event_impl.cpp

+        }
+      }
+
+      Q.pop_back();


Could you, please, describe why the last element is being popped here prior to reset sequence?

In the while (!Q.back().is_end()) one gets to the deepest element of the graph.
This element is the one without any descendants (dependencies). Hence, one should reset it then get back one step and continue digging into graph on the next dependency item.

DepIter, which contains parent for the deepest element (let it be D), returns shared pointer to D. When we need to reset D, we must lift up to its parent and reset it. I do it in the code below, after that I increment DepIter to continue exploring descendants

romanovvlad · 2021-10-26T14:43:16Z

sycl/source/detail/event_impl.cpp

+        (*Q.back()).get()->MPreparedDepsEvents.clear();
+        (*Q.back()).get()->MPreparedHostDepsEvents.clear();
+        (*Q.back()).reset();


I'm not sure I can fully understand the code, but can't we have a situation when we clearing MPreparedDepsEvents of an event_impl which is still used?

I believe, that two following implications are true:

an event (an instance of event_impl) is being destroyed once it's command is complete

a command can be complete if and only if all of its dependencies are complete

These implications make this behaviour of destructor valid.

an event (an instance of event_impl) is being destroyed once it's command is complete

An instance of event_impl is being destroyed once there is no event which points to it, so a user can hold an event even after a Command is completed and destroyed.

Anyway, probably the code is still valid in a sense that we are not required to return already completed events in get_wait_list , so we can clear wait list of a completed command(which probably even better option that one I suggested below). But access to this event lists should be guarded by a mutex in all cases.

romanovvlad · 2021-10-26T14:48:27Z

When event_impl's d-tor is called its dependencies are starting to recursive releasing. It leads to stack overflow. Add manual cleaning of dependencies by DFS to event_impl's d-tor to avoid this problem. Signed-off-by: mdimakov [email protected]

As an alternative approach we could try to update MPreparedDepsEvents of events that are connected to users of the Schduler::Command when it's deconstructing. In this option accesses to MPreparedDepsEvents should be guarded by a lock.

sycl/source/detail/scheduler/commands.cpp

romanovvlad · 2021-11-10T10:17:20Z

sycl/source/detail/event_impl.hpp

@@ -164,14 +164,14 @@ class event_impl {
  /// Returns vector of event dependencies.
  ///
  /// @return a reference to MPreparedDepsEvents.
-  std::vector<std::shared_ptr<event_impl>> &getPreparedDepsEvents() {
+  std::vector<std::weak_ptr<event_impl>> &getPreparedDepsEvents() {


Could you please tell who own/holds event_impl with this patch?

Still think that approach mentioned in this comment would be much simpler #4793 (comment).

s-kanaev · 2021-11-11T11:08:55Z

sycl/source/detail/event_impl.hpp


  enum HostEventState : int { HES_NotComplete = 0, HES_Complete };

  // State of host event. Employed only for host events and event with no
  // backend's representation (e.g. alloca). Used values are listed in
  // HostEventState enum.
  std::atomic<int> MState;
+
+  std::mutex MMutex;


Why is this mutex needed? Is access to MPreparedDepsEvents and MPreparedHostDepsEvents not guarded by graph R/W lock?

Access to these dependencies is not guarded when the user wants to get them from event::get_wait_list

s-kanaev · 2021-11-11T11:10:17Z

sycl/source/detail/scheduler/graph_builder.cpp

@@ -129,6 +129,7 @@ static void handleVisitedNodes(std::vector<Command *> &Visited) {
  for (Command *Cmd : Visited) {
    if (Cmd->MMarks.MToBeDeleted) {
      Cmd->getEvent()->setCommand(nullptr);
+      Cmd->getEvent()->cleanupDependencyEvents();


This seems to still lead to d-tor to go through DFS and overflowing the stack, isn't it?

Last changes are not lead to stack overflow on the tests that were affected

romanovvlad · 2021-11-12T11:46:21Z

sycl/source/detail/event_impl.cpp

@@ -217,8 +217,7 @@ void event_impl::wait_and_throw(



Not related to this patch, but it seems this function is not thread safe. We accessing Cmd here which AFAIK can be removed in graph cleanup.

I wrapped wait_and_throw under mutex

s-kanaev · 2021-11-14T15:08:56Z

sycl/source/detail/event_impl.cpp

@@ -210,15 +210,17 @@ void event_impl::wait(

 void event_impl::wait_and_throw(
    std::shared_ptr<cl::sycl::detail::event_impl> Self) {
+  Scheduler &Sched = Scheduler::getInstance();
+  Scheduler::ReadLockT Lock(Sched.MGraphLock);
+
  Command *Cmd = static_cast<Command *>(Self->getCommand());
  QueueImplPtr submittedQueue = nullptr;
  if (Cmd)
    submittedQueue = Cmd->getSubmittedQueue();

  wait(Self);


Call to wait should not take place with graph lock taken.

s-kanaev

Seems legit.

s-kanaev · 2021-11-15T09:50:28Z

@maximdimakov , could you, please, add a test for this fix in intel/llvm-test-suite?

dm-vodopyanov · 2021-11-18T11:03:25Z

@maximdimakov , can you add a link to PR with tests for this patch from intel/llvm-test-suite?

Manual cleaning of event_impl dependency trees

b46adfe

Signed-off-by: mdimakov <[email protected]>

maximdimakov requested a review from s-kanaev October 21, 2021 10:02

maximdimakov requested a review from a team as a code owner October 21, 2021 10:02

s-kanaev reviewed Oct 21, 2021

View reviewed changes

maximdimakov added 5 commits October 25, 2021 13:31

Implement DFS

bdb43b8

Remove comments

1ccf2b1

Format fixes

1baab21

Clang-format fix

a5153d6

Clang-format fix

00bef9e

maximdimakov requested a review from s-kanaev October 25, 2021 10:49

Merge remote-tracking branch 'origin/sycl' into fix_dependency_release

a9dfd7c

s-kanaev reviewed Oct 26, 2021

View reviewed changes

romanovvlad reviewed Oct 26, 2021

View reviewed changes

Merge remote-tracking branch 'origin/sycl' into fix_dependency_release

941b469

maximdimakov marked this pull request as draft November 9, 2021 12:00

Replace shared_ptr to weak_ptr

ee3502f

s-kanaev reviewed Nov 9, 2021

View reviewed changes

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

maximdimakov added 2 commits November 10, 2021 11:17

Format fixes

b918571

Format fixes

2f5b026

maximdimakov changed the title ~~[SYCL] Add manual cleaning of event_impl dependency trees in d-tor~~ [SYCL] Holding prepared events in weak_ptr instead of shared_ptr Nov 10, 2021

maximdimakov changed the title ~~[SYCL] Holding prepared events in weak_ptr instead of shared_ptr~~ [SYCL] Keep prepared events in weak_ptr instead of shared_ptr Nov 10, 2021

romanovvlad reviewed Nov 10, 2021

View reviewed changes

maximdimakov added 2 commits November 11, 2021 12:17

Working draft

d04461f

Clang-format fix

dfcba47

s-kanaev reviewed Nov 11, 2021

View reviewed changes

maximdimakov changed the title ~~[SYCL] Keep prepared events in weak_ptr instead of shared_ptr~~ [SYCL] Clear event_impl dependencies with graph cleanup Nov 11, 2021

maximdimakov marked this pull request as ready for review November 12, 2021 09:00

romanovvlad reviewed Nov 12, 2021

View reviewed changes

Wrap access to cmd under read mutex

3e3803a

maximdimakov requested review from s-kanaev and romanovvlad November 14, 2021 13:58

s-kanaev suggested changes Nov 14, 2021

View reviewed changes

maximdimakov added 2 commits November 14, 2021 18:22

Take mutex only when command is accessed

b56a925

Clang--format fix

2727036

maximdimakov requested a review from s-kanaev November 15, 2021 08:03

s-kanaev approved these changes Nov 15, 2021

View reviewed changes

maximdimakov mentioned this pull request Nov 19, 2021

[SYCL] Test for event destruction intel/llvm-test-suite#574

Merged

dm-vodopyanov merged commit 5bb3ab9 into intel:sycl Nov 19, 2021

maximdimakov deleted the fix_dependency_release branch March 24, 2022 13:59

[SYCL] Clear event_impl dependencies with graph cleanup #4793

[SYCL] Clear event_impl dependencies with graph cleanup #4793

Uh oh!

Conversation

maximdimakov commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximdimakov commented Oct 21, 2021

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romanovvlad commented Oct 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

s-kanaev commented Nov 15, 2021

Uh oh!

dm-vodopyanov commented Nov 18, 2021

Uh oh!

Uh oh!

maximdimakov commented Oct 21, 2021 •

edited

Loading