[SYCL] Fix memory leak in reduction resources #5162
Conversation
Reductions that require additional resources, such as buffers, can currently create a circular dependency between the resources and the commands issued by the reductions. These changes clear up this dependence in a similar way to how streams are transferred by transferring ownership of the resources to the commands and ensuring release when cleaning up the commands. Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
|
/verify with intel/llvm-test-suite#624 |
| std::vector<std::shared_ptr<stream_impl>> Streams = ExecCmd->getStreams(); | ||
| ExecCmd->clearStreams(); | ||
| StreamsToDeallocate.insert(StreamsToDeallocate.end(), Streams.begin(), | ||
| Streams.end()); | ||
|
|
||
| // Transfer ownership of reduction resources. | ||
| std::vector<std::shared_ptr<const void>> ReduResources = |
There was a problem hiding this comment.
I'm currently working on moving away from finished command cleanup towards cleanup after enqueue in order to address some queue::wait related performance issues, see #5070. Since stream handling is currently tied to post wait traversal, kernels with streams still use finished command cleanup in the linked initial implementation. Host tasks are another exception, for a different reason. Once the implementation is fully complete, cleanupFinishedCommands should be dropped entirely.
With this patch CGs with reduction resources would have to be yet another temporary exception waiting for a different solution. So I would really prefer if addressing this didn't involve finished subgraph traversal.
There was a problem hiding this comment.
Sure. I will convert this to a draft while I think of something.
There was a problem hiding this comment.
I'm currently working on moving away from finished command cleanup towards cleanup after enqueue in order to address some queue::wait related performance issues, see #5070
@sergey-semenov @steffenlarsen folks, could you please if this patch is still relevant?
There was a problem hiding this comment.
With #5070 merged this should be redesigned to work with the new approach. The problem that this PR attempts to solve is still there, but the solution is no longer applicable. I will close the PR but we should still be able to discuss it here.
Reductions that require additional resources, such as buffers, can currently create a circular dependency between the resources and the commands issued by the reductions. These changes clear up this dependence in a similar way to how streams are transferred by transferring ownership of the resources to the commands and ensuring release when cleaning up the commands.
Note: This is a non-breaking ABI change. The changes to the dump are the result of running the abi-checker script, having it dump symbols. Some replicate symbols have been removed and some have been moved. The new symbols are: