Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed the memory leaks directly in execution session #1746

Merged
15 changes: 15 additions & 0 deletions src/Runtime/ExecutionSession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

#include "ExecutionSession.hpp"

void omTensorListDestroyWithoutDestroyingTensors(OMTensorList *list);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move into ExecutionSession.hpp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to expose it to anyone doing the ExecutionSession, but created a OMTensorListHelper.hpp file for that.


namespace onnx_mlir {
const std::string ExecutionSession::_queryEntryPointsName =
"omQueryEntryPoints";
Expand Down Expand Up @@ -85,6 +87,13 @@ std::vector<OMTensorUniquePtr> ExecutionSession::run(
auto *wrappedInput = omTensorListCreate(&omts[0], (int64_t)omts.size());

auto *wrappedOutput = _entryPointFunc(wrappedInput);

// We created a wrapper for the input list, but the input list does not really
// own the tensor in the list, as they are coming as OMTensorUniquePtr. So we
// need to simply deallocate the list structure without touching the
// OMTensors.
omTensorListDestroyWithoutDestroyingTensors(wrappedInput);

if (!wrappedOutput)
throw std::runtime_error(reportErrnoError());
std::vector<OMTensorUniquePtr> outs;
Expand All @@ -93,6 +102,12 @@ std::vector<OMTensorUniquePtr> ExecutionSession::run(
outs.emplace_back(OMTensorUniquePtr(
omTensorListGetOmtByIndex(wrappedOutput, i), omTensorDestroy));
}

// We created a wrapper for the output list, but the output list does not
// really own the tensor in the list, as they are returned in a vector of
// OMTensorUniquePtr. So we need to simply deallocate the list structure
// without touching the OMTensors.
omTensorListDestroyWithoutDestroyingTensors(wrappedOutput);
errno = 0; // No errors.
return outs;
}
Expand Down
3 changes: 2 additions & 1 deletion src/Runtime/OMTensor.inc
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,9 @@ OMTensor *omTensorCreateEmpty(
void omTensorDestroy(OMTensor *tensor) {
if (!tensor)
return;
if (tensor->_owning)
if (tensor->_owning) {
free(tensor->_allocatedPtr);
}
free(tensor->_shape);
free(tensor->_strides);
free(tensor);
Expand Down
17 changes: 16 additions & 1 deletion src/Runtime/OMTensorList.inc
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,23 @@ void omTensorListDestroy(OMTensorList *list) {
return;
for (int64_t i = 0; i < list->_size; i++)
omTensorDestroy(list->_omts[i]);
if (list->_owning)
if (list->_owning) {
free(list->_omts);
}
free(list);
}

/* OMTensorList destroyer which does not destroy the tensors. Currently an
* unpublished call that is mainly used to handle the unique tensor scheme used
* in Execution Session.
*/
void omTensorListDestroyWithoutDestroyingTensors(OMTensorList *list) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe shorter name omTensorListDestroyShallow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like it better too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (!list)
return;
// Omit destruction of the OMTensors.
if (list->_owning) {
free(list->_omts);
}
free(list);
}

Expand Down
8 changes: 4 additions & 4 deletions test/numerical/TestLoop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ bool isOMLoopTheSameAsNaiveImplFor(std::string moduleIR,
omTensorGetElem<bool>(condTensor.get(), {}) = true;
inputs.emplace_back(move(condTensor));

auto *yInitShape = new int64_t[1]{1};
int64_t yInitShape = 1;
auto yInitTensor = OMTensorUniquePtr(
omTensorCreateEmpty(&yInitShape[0], 1, OM_DATA_TYPE::ONNX_TYPE_INT64),
omTensorCreateEmpty(&yInitShape, 1, OM_DATA_TYPE::ONNX_TYPE_INT64),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can avoid changing this line by changing the declaration for yInitShape to:

int64_t yInitShape[1] = { 1 };

Same for yRefInitShape below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the difference, but yes we can. done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Difference is the array definition serves as a reminder that shape is typically an array.

Copy link
Collaborator Author

@AlexandreEichenberger AlexandreEichenberger Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I buy that even though there is no difference between a scalar and an array of 1 element.

omTensorDestroy);
omTensorGetElem<int64_t>(yInitTensor.get(), {0}) = yInit;
inputs.emplace_back(move(yInitTensor));
Expand All @@ -135,9 +135,9 @@ bool isOMLoopTheSameAsNaiveImplFor(std::string moduleIR,
return false;
}

auto *yRefInitShape = new int64_t[1]{1};
int64_t yRefInitShape = 1;
chentong319 marked this conversation as resolved.
Show resolved Hide resolved
auto vFinalRef = OMTensorUniquePtr(
omTensorCreateEmpty(&yRefInitShape[0], 1, OM_DATA_TYPE::ONNX_TYPE_INT64),
omTensorCreateEmpty(&yRefInitShape, 1, OM_DATA_TYPE::ONNX_TYPE_INT64),
omTensorDestroy);

omTensorGetElem<int64_t>(vFinalRef.get(), {0}) = yInit;
Expand Down