[SYCL] Store stream buffers in the scheduler #2416

againull · 2020-09-03T08:59:51Z

Stream buffers need to be alive after submitting a kernel because it is
executed by the scheduler asynchronously. For this reason currently
stream buffers are stored in an associated stream object. This stream
object is passed to the handler and then forwarded further to a command
group to keep stream buffers alive for the scheduler.

But there is a problem with this approach. A command group cannot be
destroyed while stream buffers (which are accessed in this command
group) are alive. Stream buffers are destroyed only if the stream
is destroyed. Stream object is destroyed only if command group is
destroyed. So, there is a loop dependency. Which results in memory leaks.

Solution is to store stream buffers in the scheduler for each stream.
With this approach resources are released properly.

Stream buffers need to be alive after submitting a kernel because it is executed by the scheduler asynchronosly. For this reason currently stream buffers are stored in an associated stream object. This stream object is passed to the handler and then forwarded further to a commandi group to keep streamm buffers alive for the scheduler. But there is a problem with this approach. A command group cannot be destroyed while stream buffers (which are accessed in this command group) are alive. Stream buffers are destroyed only if the stream is destroyed. Stream object is destrtoyed only if command group is destroyed. So, there is a loop dependcy. Which results in memory leaks. Solution is to store stream buffers in the scheduler for each stream. With this approach resources are released properly.

s-kanaev

Looks good overall.
A couple of comments only.
Some LIT tests failed also.

s-kanaev · 2020-09-03T15:18:21Z

sycl/source/detail/scheduler/scheduler.hpp

@@ -715,6 +715,38 @@ class Scheduler {

  friend class Command;
  friend class DispatchHostTask;
+
+  class StreamBuffers {


We need at least some info on this class.

Added info in the style of this file

s-kanaev · 2020-09-03T15:18:50Z

sycl/source/detail/scheduler/scheduler.hpp

@@ -715,6 +715,38 @@ class Scheduler {

  friend class Command;
  friend class DispatchHostTask;
+
+  class StreamBuffers {


It's all public. Why is it class then? Suggest switching to struct here.

Agree, switched to structure

sycl/source/detail/stream_impl.cpp

s-kanaev · 2020-09-03T15:30:56Z

sycl/test/basic_tests/stream/release_resources_test.cpp

+//==----------------------- release_resources_test.cpp ---------------------==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//


I think this part isn't needed here.

againull

Also fixed CI fails (hopefully testing will pass). Problem is that scheduler is a static object. And looks like some runtimes (for example, cuda, cpu) are unloaded before destructor of the scheduler is called. When buffers are deleted we at least call wait() for events in the scheduler, so this is a problem if device runtime is unloaded.
That is why I added changes to remove buffers for a stream object as soon as stream is flushed. We can do this because flushing is a blocking operation at this moment but this is going to be changed in the future.

sycl/source/detail/stream_impl.cpp

againull · 2020-09-04T06:27:04Z

sycl/source/detail/scheduler/scheduler.hpp

@@ -715,6 +715,38 @@ class Scheduler {

  friend class Command;
  friend class DispatchHostTask;
+
+  class StreamBuffers {


Added info in the style of this file

againull · 2020-09-04T06:27:09Z

sycl/source/detail/scheduler/scheduler.hpp

@@ -715,6 +715,38 @@ class Scheduler {

  friend class Command;
  friend class DispatchHostTask;
+
+  class StreamBuffers {


Agree, switched to structure

againull · 2020-09-04T06:27:24Z

sycl/test/basic_tests/stream/release_resources_test.cpp

+//==----------------------- release_resources_test.cpp ---------------------==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//


bader · 2020-09-04T06:57:16Z

@againull, please, take a look at CI regressions.

bader · 2020-09-04T06:57:24Z

/summary:run

againull · 2020-09-04T07:02:37Z

@againull, please, take a look at CI regressions.

@bader , I am quite sure it is caused by #2409

bader · 2020-09-04T07:12:18Z

@againull, please, take a look at CI regressions.

@bader , I am quite sure it is caused by #2409

@againull, why do you think so? There are no CI failures in this PR pre-commit.
@yanfeng3721, could you confirm that #2409 introduced this regression?

againull · 2020-09-04T07:17:51Z

@againull, please, take a look at CI regressions.

@bader , I am quite sure it is caused by #2409

@againull, why do you think so? There are no CI failures in this PR pre-commit.
@yanfeng3721, could you confirm that #2409 introduced this regression?

This test FAIL: SYCL::image_accessor_readsampler.cpp failed in precommit for #2409, but adding some new changes to #2409 triggered new testing and it passed, I believe it is a flaky failure. And this test is completely unrelated to changes of this PR.

bader · 2020-09-04T07:22:26Z

@againull, @yanfeng3721, please, either revert the #2409 caused the regression or temporarily disable failing test.
Please, keep CI system sane.

yanfeng3721 · 2020-09-04T07:57:27Z

Hi , I have create a PR: #2422 to disable the flaky test temporarily .

againull · 2020-09-04T08:07:12Z

/summary:run

[L0] Disabling Driver In Order Lists by default

againull requested a review from a team as a code owner September 3, 2020 08:59

againull requested a review from s-kanaev September 3, 2020 08:59

s-kanaev reviewed Sep 3, 2020

View reviewed changes

againull added 2 commits September 3, 2020 23:20

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

cd8a698

Address review comments and fix CI fails

a0bf820

againull commented Sep 4, 2020

View reviewed changes

againull requested a review from s-kanaev September 4, 2020 06:36

s-kanaev previously approved these changes Sep 4, 2020

View reviewed changes

[SYCL] Disable flaky test after uplifting to Level Zero v1.0

4708e96

againull dismissed s-kanaev’s stale review via 4708e96 September 4, 2020 08:06

bader requested a review from s-kanaev September 4, 2020 10:59

bader merged commit 841e1e7 into intel:sycl Sep 4, 2020

againull deleted the stream_release_resources branch December 3, 2022 00:03

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2416 from nrspruit/disable_driver_inorderlists

7997c24

[L0] Disabling Driver In Order Lists by default

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Store stream buffers in the scheduler #2416

[SYCL] Store stream buffers in the scheduler #2416

againull commented Sep 3, 2020 •

edited

Loading

s-kanaev left a comment

s-kanaev Sep 3, 2020

againull Sep 4, 2020

s-kanaev Sep 3, 2020

againull Sep 4, 2020

s-kanaev Sep 3, 2020

againull Sep 4, 2020

againull left a comment •

edited

Loading

againull Sep 4, 2020

againull Sep 4, 2020

againull Sep 4, 2020

bader commented Sep 4, 2020

bader commented Sep 4, 2020

againull commented Sep 4, 2020 •

edited

Loading

bader commented Sep 4, 2020

againull commented Sep 4, 2020 •

edited

Loading

bader commented Sep 4, 2020

yanfeng3721 commented Sep 4, 2020

againull commented Sep 4, 2020

[SYCL] Store stream buffers in the scheduler #2416

[SYCL] Store stream buffers in the scheduler #2416

Conversation

againull commented Sep 3, 2020 • edited Loading

s-kanaev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

againull left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented Sep 4, 2020

bader commented Sep 4, 2020

againull commented Sep 4, 2020 • edited Loading

bader commented Sep 4, 2020

againull commented Sep 4, 2020 • edited Loading

bader commented Sep 4, 2020

yanfeng3721 commented Sep 4, 2020

againull commented Sep 4, 2020

againull commented Sep 3, 2020 •

edited

Loading

againull left a comment •

edited

Loading

againull commented Sep 4, 2020 •

edited

Loading

againull commented Sep 4, 2020 •

edited

Loading