From 0a3ecf3f5d3076d79b86458b67c0508a96ea287e Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Thu, 17 Sep 2020 13:43:11 -0700 Subject: [PATCH 01/14] when a command has no mem dependencies, we now release it early, but track its event in USMEvents, guaranteeing both its completion and ultimate release Signed-off-by: Chris Perkins --- sycl/source/detail/queue_impl.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/sycl/source/detail/queue_impl.cpp b/sycl/source/detail/queue_impl.cpp index 52894ed3b9b20..1479bdf68f13d 100644 --- a/sycl/source/detail/queue_impl.cpp +++ b/sycl/source/detail/queue_impl.cpp @@ -97,9 +97,20 @@ event queue_impl::mem_advise(const shared_ptr_class &Self, } void queue_impl::addEvent(const event &Event) { - std::weak_ptr EventWeakPtr{getSyclObjImpl(Event)}; - std::lock_guard Lock(MMutex); - MEvents.push_back(std::move(EventWeakPtr)); + // if the command behind the event has no memory dependencies, + // we need to track the event with the USMEvents, or it won't be properly + // released. + EventImplPtr Eimpl = getSyclObjImpl(Event); + Command *Cmd = (Command *)(Eimpl->getCommand()); + if (Cmd && Cmd->MDeps.size() == 0) { + addUSMEvent(Event); + Eimpl->setCommand(nullptr); // decouple and free the command + delete Cmd; + } else { + std::weak_ptr EventWeakPtr{Eimpl}; + std::lock_guard Lock(MMutex); + MEvents.push_back(std::move(EventWeakPtr)); + } } void queue_impl::addUSMEvent(const event &Event) { From 11161cac0f45495522d88430f3d3658d822ae82e Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Thu, 17 Sep 2020 15:59:28 -0700 Subject: [PATCH 02/14] added lit test Signed-off-by: Chris Perkins --- sycl/test/basic_tests/queue/release.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 sycl/test/basic_tests/queue/release.cpp diff --git a/sycl/test/basic_tests/queue/release.cpp b/sycl/test/basic_tests/queue/release.cpp new file mode 100644 index 0000000000000..794623d0d26e0 --- /dev/null +++ b/sycl/test/basic_tests/queue/release.cpp @@ -0,0 +1,19 @@ +// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out +// RUN: env SYCL_PI_TRACE=2 %GPU_RUN_PLACEHOLDER %t.out | FileCheck %s + +#include +int main() { + sycl::queue q; + + q.single_task([]() {}); + // no wait. ensure resources are released anyway. + + return 0; +} + +//CHECK: ---> piEnqueueKernelLaunch( +//CHECK: ---> piQueueRelease( +//CHECK: ---> piEventRelease( +//CHECK: ---> piContextRelease( +//CHECK: ---> piKernelRelease( +//CHECK: ---> piProgramRelease( \ No newline at end of file From 53fffa05650422ecf3f5510f5c0958097625a887 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Fri, 18 Sep 2020 12:03:53 -0700 Subject: [PATCH 03/14] review feedback Signed-off-by: Chris Perkins --- sycl/source/detail/queue_impl.cpp | 19 ++++++++----------- sycl/source/detail/queue_impl.hpp | 12 ++++++++---- sycl/source/detail/scheduler/scheduler.cpp | 6 ++++++ sycl/test/basic_tests/queue/release.cpp | 2 +- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/sycl/source/detail/queue_impl.cpp b/sycl/source/detail/queue_impl.cpp index 1479bdf68f13d..0ef6331c2fdc2 100644 --- a/sycl/source/detail/queue_impl.cpp +++ b/sycl/source/detail/queue_impl.cpp @@ -97,25 +97,22 @@ event queue_impl::mem_advise(const shared_ptr_class &Self, } void queue_impl::addEvent(const event &Event) { - // if the command behind the event has no memory dependencies, - // we need to track the event with the USMEvents, or it won't be properly - // released. EventImplPtr Eimpl = getSyclObjImpl(Event); Command *Cmd = (Command *)(Eimpl->getCommand()); - if (Cmd && Cmd->MDeps.size() == 0) { + if (!Cmd) { + // if there is no command on the event, we cannot track it with MEventsWeak + // as that will leave it with no owner. Track in MEventsShared addUSMEvent(Event); - Eimpl->setCommand(nullptr); // decouple and free the command - delete Cmd; } else { std::weak_ptr EventWeakPtr{Eimpl}; - std::lock_guard Lock(MMutex); - MEvents.push_back(std::move(EventWeakPtr)); + std::lock_guard Lock{MMutex}; + MEventsWeak.push_back(std::move(EventWeakPtr)); } } void queue_impl::addUSMEvent(const event &Event) { std::lock_guard Lock(MMutex); - MUSMEvents.push_back(Event); + MEventsShared.push_back(Event); } void *queue_impl::instrumentationProlog(const detail::code_location &CodeLoc, @@ -215,8 +212,8 @@ void queue_impl::wait(const detail::code_location &CodeLoc) { vector_class USMEvents; { std::lock_guard Lock(MMutex); - Events = std::move(MEvents); - USMEvents = std::move(MUSMEvents); + Events = std::move(MEventsWeak); + USMEvents = std::move(MEventsShared); } for (std::weak_ptr &EventImplWeakPtr : Events) diff --git a/sycl/source/detail/queue_impl.hpp b/sycl/source/detail/queue_impl.hpp index 7b1bf09317bc1..29aa3dba9ceb3 100644 --- a/sycl/source/detail/queue_impl.hpp +++ b/sycl/source/detail/queue_impl.hpp @@ -415,10 +415,14 @@ class queue_impl { DeviceImplPtr MDevice; const ContextImplPtr MContext; - vector_class> MEvents; - // USM operations are not added to the scheduler command graph, - // queue is the only owner on the runtime side. - vector_class MUSMEvents; + + /// These events are tracked, but not owned, by the queue. + vector_class> MEventsWeak; + + /// Events without data dependencies (such as USM) need an owner, + /// additionally, USM operations are not added to the scheduler command graph, + /// queue is the only owner on the runtime side. + vector_class MEventsShared; exception_list MExceptions; const async_handler MAsyncHandler; const property_list MPropList; diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index 1ca92d93080c4..6b6d97e3d70b8 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -97,6 +97,12 @@ EventImplPtr Scheduler::addCG(std::unique_ptr CommandGroup, if (!Enqueued && EnqueueResultT::SyclEnqueueFailed == Res.MResult) throw runtime_error("Enqueue process failed.", PI_INVALID_OPERATION); + if (NewCmd->MDeps.size() == 0) { + NewEvent->setCommand(nullptr); // if there are no memory dependencies, + // decouple and free the command + delete NewCmd; + } + if (IsKernel) Streams = ((ExecCGCommand *)NewCmd)->getStreams(); } diff --git a/sycl/test/basic_tests/queue/release.cpp b/sycl/test/basic_tests/queue/release.cpp index 794623d0d26e0..f1cf28cc75827 100644 --- a/sycl/test/basic_tests/queue/release.cpp +++ b/sycl/test/basic_tests/queue/release.cpp @@ -16,4 +16,4 @@ int main() { //CHECK: ---> piEventRelease( //CHECK: ---> piContextRelease( //CHECK: ---> piKernelRelease( -//CHECK: ---> piProgramRelease( \ No newline at end of file +//CHECK: ---> piProgramRelease( From 8895569539b4c3fef843d77b998c075edbbabd53 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Fri, 18 Sep 2020 15:19:20 -0700 Subject: [PATCH 04/14] re-run ci Signed-off-by: Chris Perkins --- sycl/test/basic_tests/queue/release.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/test/basic_tests/queue/release.cpp b/sycl/test/basic_tests/queue/release.cpp index f1cf28cc75827..c92086e5c3574 100644 --- a/sycl/test/basic_tests/queue/release.cpp +++ b/sycl/test/basic_tests/queue/release.cpp @@ -6,7 +6,7 @@ int main() { sycl::queue q; q.single_task([]() {}); - // no wait. ensure resources are released anyway. + // no wait. Ensure resources are released anyway. return 0; } From 995a8d29f4a05b29d6d7dba25ff2d262037fc237 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Mon, 21 Sep 2020 09:39:07 -0700 Subject: [PATCH 05/14] re-run ci? Signed-off-by: Chris Perkins --- sycl/test/basic_tests/queue/release.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/test/basic_tests/queue/release.cpp b/sycl/test/basic_tests/queue/release.cpp index c92086e5c3574..2c38042e555fb 100644 --- a/sycl/test/basic_tests/queue/release.cpp +++ b/sycl/test/basic_tests/queue/release.cpp @@ -6,7 +6,7 @@ int main() { sycl::queue q; q.single_task([]() {}); - // no wait. Ensure resources are released anyway. + // No wait. Ensure resources are released anyway. return 0; } From 5ff396cff5bbe44451a84151efdec82a8e874e52 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Wed, 23 Sep 2020 12:43:20 -0700 Subject: [PATCH 06/14] tracked down mem instability to over-eager deletion. Don't need to delete for host queue. Signed-off-by: Chris Perkins --- sycl/source/detail/scheduler/scheduler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index 6b6d97e3d70b8..c1e22deefcdfd 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -97,7 +97,7 @@ EventImplPtr Scheduler::addCG(std::unique_ptr CommandGroup, if (!Enqueued && EnqueueResultT::SyclEnqueueFailed == Res.MResult) throw runtime_error("Enqueue process failed.", PI_INVALID_OPERATION); - if (NewCmd->MDeps.size() == 0) { + if (NewCmd->MDeps.size() == 0 && NewCmd->getQueue() != DefaultHostQueue) { NewEvent->setCommand(nullptr); // if there are no memory dependencies, // decouple and free the command delete NewCmd; From 44fcc2ce00677b3f14bab238964d7421a6bcea37 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Wed, 23 Sep 2020 14:57:48 -0700 Subject: [PATCH 07/14] rename routine. addUSMEvent => addSharedEvent Signed-off-by: Chris Perkins --- sycl/source/detail/queue_impl.cpp | 13 ++++++++----- sycl/source/detail/queue_impl.hpp | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/sycl/source/detail/queue_impl.cpp b/sycl/source/detail/queue_impl.cpp index 0ef6331c2fdc2..d5aeae29c1195 100644 --- a/sycl/source/detail/queue_impl.cpp +++ b/sycl/source/detail/queue_impl.cpp @@ -60,7 +60,7 @@ event queue_impl::memset(const shared_ptr_class &Self, return event(); event ResEvent = prepareUSMEvent(Self, NativeEvent); - addUSMEvent(ResEvent); + addSharedEvent(ResEvent); return ResEvent; } @@ -74,7 +74,7 @@ event queue_impl::memcpy(const shared_ptr_class &Self, return event(); event ResEvent = prepareUSMEvent(Self, NativeEvent); - addUSMEvent(ResEvent); + addSharedEvent(ResEvent); return ResEvent; } @@ -92,7 +92,7 @@ event queue_impl::mem_advise(const shared_ptr_class &Self, Advice, &NativeEvent); event ResEvent = prepareUSMEvent(Self, NativeEvent); - addUSMEvent(ResEvent); + addSharedEvent(ResEvent); return ResEvent; } @@ -102,7 +102,7 @@ void queue_impl::addEvent(const event &Event) { if (!Cmd) { // if there is no command on the event, we cannot track it with MEventsWeak // as that will leave it with no owner. Track in MEventsShared - addUSMEvent(Event); + addSharedEvent(Event); } else { std::weak_ptr EventWeakPtr{Eimpl}; std::lock_guard Lock{MMutex}; @@ -110,7 +110,10 @@ void queue_impl::addEvent(const event &Event) { } } -void queue_impl::addUSMEvent(const event &Event) { +/// addSharedEvent - queue_impl tracks events with weak pointers +/// but some events have no other owner. In this case, +/// addSharedEvent will have the queue track the events via a shared pointer. +void queue_impl::addSharedEvent(const event &Event) { std::lock_guard Lock(MMutex); MEventsShared.push_back(Event); } diff --git a/sycl/source/detail/queue_impl.hpp b/sycl/source/detail/queue_impl.hpp index 29aa3dba9ceb3..49dba989adaef 100644 --- a/sycl/source/detail/queue_impl.hpp +++ b/sycl/source/detail/queue_impl.hpp @@ -403,7 +403,7 @@ class queue_impl { /// Stores a USM operation event that should be associated with the queue /// /// \param Event is the event to be stored - void addUSMEvent(const event &Event); + void addSharedEvent(const event &Event); /// Stores an event that should be associated with the queue /// From 99d25a00ee2698227fb15aa1dfc099f5d43bbfec Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Fri, 25 Sep 2020 13:40:13 -0700 Subject: [PATCH 08/14] ci test Signed-off-by: Chris Perkins --- sycl/test/basic_tests/queue/release.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/test/basic_tests/queue/release.cpp b/sycl/test/basic_tests/queue/release.cpp index 2c38042e555fb..c92086e5c3574 100644 --- a/sycl/test/basic_tests/queue/release.cpp +++ b/sycl/test/basic_tests/queue/release.cpp @@ -6,7 +6,7 @@ int main() { sycl::queue q; q.single_task([]() {}); - // No wait. Ensure resources are released anyway. + // no wait. Ensure resources are released anyway. return 0; } From 0ac6f4129711a984bc0b5a3d119a5f1d14041b41 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Mon, 28 Sep 2020 17:16:55 -0700 Subject: [PATCH 09/14] ci test Signed-off-by: Chris Perkins --- sycl/source/detail/scheduler/scheduler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index c1e22deefcdfd..04a2e82f027cf 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -97,7 +97,7 @@ EventImplPtr Scheduler::addCG(std::unique_ptr CommandGroup, if (!Enqueued && EnqueueResultT::SyclEnqueueFailed == Res.MResult) throw runtime_error("Enqueue process failed.", PI_INVALID_OPERATION); - if (NewCmd->MDeps.size() == 0 && NewCmd->getQueue() != DefaultHostQueue) { + if (NewCmd->MDeps.size() == 0 && NewCmd->MUsers.size() == 0) { NewEvent->setCommand(nullptr); // if there are no memory dependencies, // decouple and free the command delete NewCmd; From e5833042035196a363ba9f69224a13fd8be43893 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Mon, 28 Sep 2020 19:04:21 -0700 Subject: [PATCH 10/14] verify Signed-off-by: Chris Perkins --- sycl/test/basic_tests/queue/release.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/test/basic_tests/queue/release.cpp b/sycl/test/basic_tests/queue/release.cpp index c92086e5c3574..2c38042e555fb 100644 --- a/sycl/test/basic_tests/queue/release.cpp +++ b/sycl/test/basic_tests/queue/release.cpp @@ -6,7 +6,7 @@ int main() { sycl::queue q; q.single_task([]() {}); - // no wait. Ensure resources are released anyway. + // No wait. Ensure resources are released anyway. return 0; } From 95e9e39ea62d176205f310da5e56bdee6ea7392f Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Tue, 29 Sep 2020 09:00:14 -0700 Subject: [PATCH 11/14] updated comment after losing short bout to dyslexia Signed-off-by: Chris Perkins --- sycl/source/detail/queue_impl.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sycl/source/detail/queue_impl.hpp b/sycl/source/detail/queue_impl.hpp index 49dba989adaef..473fad6500b2d 100644 --- a/sycl/source/detail/queue_impl.hpp +++ b/sycl/source/detail/queue_impl.hpp @@ -400,7 +400,9 @@ class queue_impl { void initHostTaskAndEventCallbackThreadPool(); - /// Stores a USM operation event that should be associated with the queue + /// queue_impl.addSharedEvent tracks events with weak pointers + /// but some events have no other owners. addSharedEvent() + /// follows events with a shared pointer. /// /// \param Event is the event to be stored void addSharedEvent(const event &Event); From fdcda6aaf7feb8eadaaa58a3dcdd84f425d18fc6 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Tue, 29 Sep 2020 14:43:49 -0700 Subject: [PATCH 12/14] ci test Signed-off-by: Chris Perkins --- sycl/test/basic_tests/queue/release.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/test/basic_tests/queue/release.cpp b/sycl/test/basic_tests/queue/release.cpp index 2c38042e555fb..c92086e5c3574 100644 --- a/sycl/test/basic_tests/queue/release.cpp +++ b/sycl/test/basic_tests/queue/release.cpp @@ -6,7 +6,7 @@ int main() { sycl::queue q; q.single_task([]() {}); - // No wait. Ensure resources are released anyway. + // no wait. Ensure resources are released anyway. return 0; } From 349b81dec78d6ebf5866d55f6d60e78b83bdf048 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Wed, 30 Sep 2020 09:11:12 -0700 Subject: [PATCH 13/14] comment update Signed-off-by: Chris Perkins --- sycl/source/detail/queue_impl.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/source/detail/queue_impl.hpp b/sycl/source/detail/queue_impl.hpp index 473fad6500b2d..8147b205ff29c 100644 --- a/sycl/source/detail/queue_impl.hpp +++ b/sycl/source/detail/queue_impl.hpp @@ -400,7 +400,7 @@ class queue_impl { void initHostTaskAndEventCallbackThreadPool(); - /// queue_impl.addSharedEvent tracks events with weak pointers + /// queue_impl.addEvent tracks events with weak pointers /// but some events have no other owners. addSharedEvent() /// follows events with a shared pointer. /// From 9b94c8cd922fbe1fdb00aad5d9cbb52e5ebb22c0 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Wed, 30 Sep 2020 12:06:22 -0700 Subject: [PATCH 14/14] reversal. Seems obvious in retrospect. But why lit tests not catching this reliably? Signed-off-by: Chris Perkins --- sycl/source/detail/scheduler/scheduler.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index 8550f8d194784..8f6ae7b12fad3 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -99,14 +99,14 @@ EventImplPtr Scheduler::addCG(std::unique_ptr CommandGroup, if (!Enqueued && EnqueueResultT::SyclEnqueueFailed == Res.MResult) throw runtime_error("Enqueue process failed.", PI_INVALID_OPERATION); + if (IsKernel) + Streams = ((ExecCGCommand *)NewCmd)->getStreams(); + if (NewCmd->MDeps.size() == 0 && NewCmd->MUsers.size() == 0) { NewEvent->setCommand(nullptr); // if there are no memory dependencies, // decouple and free the command delete NewCmd; } - - if (IsKernel) - Streams = ((ExecCGCommand *)NewCmd)->getStreams(); } }