Skip to content

Commit bb0261c

Browse files
steffenlarsensteffenlarsen
authored
Revert "[SYCL] Fix memory dependency leaks caused by failed kernel enqueue (#5120)" (#5718)
This reverts commit 104e439. #5120 is causing post-commit failures and performance regressions. This reverts it.
1 parent e5e57b6 commit bb0261c

File tree

6 files changed

+48
-291
lines changed

6 files changed

+48
-291
lines changed

‎sycl/source/detail/scheduler/graph_builder.cpp

Lines changed: 6 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,13 @@ MemObjRecord *Scheduler::GraphBuilder::getOrInsertMemObjRecord(
234234

235235
void Scheduler::GraphBuilder::updateLeaves(const std::set<Command *> &Cmds,
236236
MemObjRecord *Record,
237+
access::mode AccessMode,
237238
std::vector<Command *> &ToCleanUp) {
239+
240+
const bool ReadOnlyReq = AccessMode == access::mode::read;
241+
if (ReadOnlyReq)
242+
return;
243+
238244
for (Command *Cmd : Cmds) {
239245
bool WasLeaf = Cmd->MLeafCounter > 0;
240246
Cmd->MLeafCounter -= Record->MReadLeaves.remove(Cmd);
@@ -246,18 +252,6 @@ void Scheduler::GraphBuilder::updateLeaves(const std::set<Command *> &Cmds,
246252
}
247253
}
248254

249-
void Scheduler::GraphBuilder::updateLeaves(const std::set<Command *> &Cmds,
250-
MemObjRecord *Record,
251-
access::mode AccessMode,
252-
std::vector<Command *> &ToCleanUp) {
253-
254-
const bool ReadOnlyReq = AccessMode == access::mode::read;
255-
if (ReadOnlyReq)
256-
return;
257-
258-
updateLeaves(Cmds, Record, ToCleanUp);
259-
}
260-
261255
void Scheduler::GraphBuilder::addNodeToLeaves(
262256
MemObjRecord *Record, Command *Cmd, access::mode AccessMode,
263257
std::vector<Command *> &ToEnqueue) {
@@ -1259,60 +1253,6 @@ void Scheduler::GraphBuilder::cleanupFinishedCommands(
12591253
handleVisitedNodes(MVisitedCmds);
12601254
}
12611255

1262-
void Scheduler::GraphBuilder::cleanupFailedCommand(
1263-
Command *FailedCmd,
1264-
std::vector<std::shared_ptr<cl::sycl::detail::stream_impl>>
1265-
&StreamsToDeallocate,
1266-
std::vector<Command *> &ToCleanUp) {
1267-
1268-
// If the failed command has no users and no dependencies, there is no reason
1269-
// to replace it with an empty command.
1270-
if (FailedCmd->MDeps.size() == 0 && FailedCmd->MUsers.size() == 0)
1271-
return;
1272-
1273-
// Create empty command that is "ready" for enqueuing.
1274-
EmptyCommand *EmptyCmd = new EmptyCommand(FailedCmd->getQueue());
1275-
if (!EmptyCmd)
1276-
throw runtime_error("Out of host memory", PI_OUT_OF_HOST_MEMORY);
1277-
EmptyCmd->MEnqueueStatus = EnqueueResultT::SyclEnqueueReady;
1278-
1279-
// Collect stream objects for the failed command.
1280-
if (FailedCmd->getType() == Command::CommandType::RUN_CG) {
1281-
auto ExecCmd = static_cast<ExecCGCommand *>(FailedCmd);
1282-
std::vector<std::shared_ptr<stream_impl>> Streams = ExecCmd->getStreams();
1283-
ExecCmd->clearStreams();
1284-
StreamsToDeallocate.insert(StreamsToDeallocate.end(), Streams.begin(),
1285-
Streams.end());
1286-
}
1287-
1288-
for (DepDesc &Dep : FailedCmd->MDeps) {
1289-
// Replace failed command in dependency records.
1290-
const Requirement *Req = Dep.MDepRequirement;
1291-
MemObjRecord *Record = getMemObjRecord(Req->MSYCLMemObj);
1292-
updateLeaves({FailedCmd}, Record, ToCleanUp);
1293-
std::vector<Command *> ToEnqueue;
1294-
addNodeToLeaves(Record, EmptyCmd, Req->MAccessMode, ToEnqueue);
1295-
assert(ToEnqueue.empty());
1296-
1297-
// Replace failed command as a user.
1298-
if (Dep.MDepCommand->MUsers.erase(FailedCmd)) {
1299-
Dep.MDepCommand->MUsers.insert(EmptyCmd);
1300-
EmptyCmd->MDeps.push_back(Dep);
1301-
}
1302-
}
1303-
FailedCmd->MDeps.clear();
1304-
1305-
for (Command *UserCmd : FailedCmd->MUsers)
1306-
for (DepDesc &Dep : UserCmd->MDeps)
1307-
if (Dep.MDepCommand == FailedCmd)
1308-
Dep.MDepCommand = EmptyCmd;
1309-
std::swap(FailedCmd->MUsers, EmptyCmd->MUsers);
1310-
1311-
FailedCmd->getEvent()->setCommand(EmptyCmd);
1312-
assert(FailedCmd->MLeafCounter == 0);
1313-
delete FailedCmd;
1314-
}
1315-
13161256
void Scheduler::GraphBuilder::removeRecordForMemObj(SYCLMemObjI *MemObject) {
13171257
const auto It = std::find_if(
13181258
MMemObjs.begin(), MMemObjs.end(),

‎sycl/source/detail/scheduler/leaves_collection.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ static inline bool doOverlap(const Requirement *LHS, const Requirement *RHS) {
3232

3333
static inline bool isHostAccessorCmd(Command *Cmd) {
3434
return Cmd->getType() == Command::EMPTY_TASK &&
35-
Cmd->MEnqueueStatus == EnqueueResultT::SyclEnqueueBlocked &&
3635
Cmd->MBlockReason == Command::BlockReason::HostAccessor;
3736
}
3837

‎sycl/source/detail/scheduler/scheduler.cpp

Lines changed: 42 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -70,16 +70,6 @@ void Scheduler::waitForRecordToFinish(MemObjRecord *Record,
7070
}
7171
}
7272

73-
static void deallocateStreams(
74-
std::vector<std::shared_ptr<stream_impl>> &StreamsToDeallocate) {
75-
// Deallocate buffers for stream objects of the finished commands. Iterate in
76-
// reverse order because it is the order of commands execution.
77-
for (auto StreamImplPtr = StreamsToDeallocate.rbegin();
78-
StreamImplPtr != StreamsToDeallocate.rend(); ++StreamImplPtr)
79-
detail::Scheduler::getInstance().deallocateStreamBuffers(
80-
StreamImplPtr->get());
81-
}
82-
8373
EventImplPtr Scheduler::addCG(std::unique_ptr<detail::CG> CommandGroup,
8474
QueueImplPtr Queue) {
8575
EventImplPtr NewEvent = nullptr;
@@ -121,51 +111,58 @@ EventImplPtr Scheduler::addCG(std::unique_ptr<detail::CG> CommandGroup,
121111
}
122112

123113
std::vector<Command *> ToCleanUp;
124-
try {
114+
{
125115
ReadLockT Lock(MGraphLock);
126116

127117
Command *NewCmd = static_cast<Command *>(NewEvent->getCommand());
128118

129119
EnqueueResultT Res;
130120
bool Enqueued;
131121

122+
auto CleanUp = [&]() {
123+
if (NewCmd && (NewCmd->MDeps.size() == 0 && NewCmd->MUsers.size() == 0)) {
124+
if (Type == CG::RunOnHostIntel)
125+
static_cast<ExecCGCommand *>(NewCmd)->releaseCG();
126+
127+
NewEvent->setCommand(nullptr);
128+
delete NewCmd;
129+
}
130+
};
131+
132132
for (Command *Cmd : AuxiliaryCmds) {
133133
Enqueued = GraphProcessor::enqueueCommand(Cmd, Res, ToCleanUp);
134-
if (!Enqueued && EnqueueResultT::SyclEnqueueFailed == Res.MResult)
135-
throw runtime_error("Auxiliary enqueue process failed.",
136-
PI_INVALID_OPERATION);
134+
try {
135+
if (!Enqueued && EnqueueResultT::SyclEnqueueFailed == Res.MResult)
136+
throw runtime_error("Auxiliary enqueue process failed.",
137+
PI_INVALID_OPERATION);
138+
} catch (...) {
139+
// enqueueCommand() func and if statement above may throw an exception,
140+
// so destroy required resources to avoid memory leak
141+
CleanUp();
142+
std::rethrow_exception(std::current_exception());
143+
}
137144
}
138145

139146
if (NewCmd) {
140147
// TODO: Check if lazy mode.
141148
EnqueueResultT Res;
142-
bool Enqueued = GraphProcessor::enqueueCommand(NewCmd, Res, ToCleanUp);
143-
if (!Enqueued && EnqueueResultT::SyclEnqueueFailed == Res.MResult)
144-
throw runtime_error("Enqueue process failed.", PI_INVALID_OPERATION);
149+
try {
150+
bool Enqueued = GraphProcessor::enqueueCommand(NewCmd, Res, ToCleanUp);
151+
if (!Enqueued && EnqueueResultT::SyclEnqueueFailed == Res.MResult)
152+
throw runtime_error("Enqueue process failed.", PI_INVALID_OPERATION);
153+
} catch (...) {
154+
// enqueueCommand() func and if statement above may throw an exception,
155+
// so destroy required resources to avoid memory leak
156+
CleanUp();
157+
std::rethrow_exception(std::current_exception());
158+
}
145159

146160
// If there are no memory dependencies decouple and free the command.
147161
// Though, dismiss ownership of native kernel command group as it's
148162
// resources may be in use by backend and synchronization point here is
149163
// at native kernel execution finish.
150-
if (NewCmd && (NewCmd->MDeps.size() == 0 && NewCmd->MUsers.size() == 0)) {
151-
if (Type == CG::RunOnHostIntel)
152-
static_cast<ExecCGCommand *>(NewCmd)->releaseCG();
153-
154-
NewEvent->setCommand(nullptr);
155-
delete NewCmd;
156-
}
164+
CleanUp();
157165
}
158-
} catch (...) {
159-
std::vector<StreamImplPtr> StreamsToDeallocate;
160-
Command *NewCmd = static_cast<Command *>(NewEvent->getCommand());
161-
if (NewCmd) {
162-
WriteLockT Lock(MGraphLock, std::defer_lock);
163-
MGraphBuilder.cleanupFailedCommand(NewCmd, StreamsToDeallocate,
164-
ToCleanUp);
165-
}
166-
deallocateStreams(StreamsToDeallocate);
167-
cleanupCommands(ToCleanUp);
168-
std::rethrow_exception(std::current_exception());
169166
}
170167
cleanupCommands(ToCleanUp);
171168

@@ -226,6 +223,16 @@ void Scheduler::waitForEvent(EventImplPtr Event) {
226223
cleanupCommands(ToCleanUp);
227224
}
228225

226+
static void deallocateStreams(
227+
std::vector<std::shared_ptr<stream_impl>> &StreamsToDeallocate) {
228+
// Deallocate buffers for stream objects of the finished commands. Iterate in
229+
// reverse order because it is the order of commands execution.
230+
for (auto StreamImplPtr = StreamsToDeallocate.rbegin();
231+
StreamImplPtr != StreamsToDeallocate.rend(); ++StreamImplPtr)
232+
detail::Scheduler::getInstance().deallocateStreamBuffers(
233+
StreamImplPtr->get());
234+
}
235+
229236
void Scheduler::cleanupFinishedCommands(EventImplPtr FinishedEvent) {
230237
// We are going to traverse a graph of finished commands. Gather stream
231238
// objects from these commands if any and deallocate buffers for these stream

‎sycl/source/detail/scheduler/scheduler.hpp

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -516,13 +516,6 @@ class Scheduler {
516516
Command *FinishedCmd,
517517
std::vector<std::shared_ptr<cl::sycl::detail::stream_impl>> &);
518518

519-
/// Replaces a failed command in the subgraph with an empty command and
520-
/// deletes the failed command.
521-
void cleanupFailedCommand(
522-
Command *FailedCmd,
523-
std::vector<std::shared_ptr<cl::sycl::detail::stream_impl>> &,
524-
std::vector<Command *> &ToCleanUp);
525-
526519
/// Reschedules the command passed using Queue provided.
527520
///
528521
/// This can lead to rescheduling of all dependent commands. This can be
@@ -558,8 +551,6 @@ class Scheduler {
558551
std::vector<Command *> &ToEnqueue);
559552

560553
/// Removes commands from leaves.
561-
void updateLeaves(const std::set<Command *> &Cmds, MemObjRecord *Record,
562-
std::vector<Command *> &ToCleanUp);
563554
void updateLeaves(const std::set<Command *> &Cmds, MemObjRecord *Record,
564555
access::mode AccessMode,
565556
std::vector<Command *> &ToCleanUp);

0 commit comments

Comments
 (0)