[SYCL] Unload libraries in jit_compiler and kernel_compiler_opencl destructors #16517

KseniyaTikhomirova · 2025-01-03T14:56:49Z

Fixes memory leaks in jit_compiler and kernel_compiler_opencl classes.
Libraries loaded to provide compilation utils have to be released.

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

uditagarwal97

LGTM. Would it be possible to also add a test covering these changes?

KseniyaTikhomirova · 2025-01-03T16:31:39Z

LGTM. Would it be possible to also add a test covering these changes?

both objects are static so not sure if it is possible to track with UT

jopperm

Makes sense, but please add a PR description and consider updating the title, which is a bit misleading because jit_compiler doesn't load ocloc but rather the sycl-jit library.

jopperm · 2025-01-06T02:24:16Z

sycl/source/detail/jit_compiler.hpp

@@ -98,6 +98,7 @@ class jit_compiler {
  CompileSYCLFuncT CompileSYCLHandle = nullptr;
  ResetConfigFuncT ResetConfigHandle = nullptr;
  AddToConfigFuncT AddToConfigHandle = nullptr;
+  void *MLibraryHandle = nullptr;


Could this be a unique pointer as well, reusing the custom deleter from the newly-added unique pointer in jit_compiler's constructor? Then we wouldn't need to duplicate the unload logic in the destructor.

Nit: The other members here don't carry the M prefix.

done in c86ef55

title is slightly updated too.

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

jopperm · 2025-01-07T23:53:58Z

sycl/source/detail/jit_compiler.cpp

+jit_compiler::~jit_compiler() {}
+


No longer needed.

done in b326642

jopperm · 2025-01-07T23:54:14Z

sycl/source/detail/jit_compiler.hpp

-  ~jit_compiler() = default;
+  ~jit_compiler();


No longer needed.

done in b326642

jopperm · 2025-01-07T23:54:33Z

sycl/source/detail/jit_compiler.hpp

-  bool Available;
+  bool Available = false;


Is this necessary?

I just noticed that there is no pre initialization for this var, I think that code analyzer could report it

jopperm · 2025-01-07T23:59:12Z

sycl/source/detail/jit_compiler.cpp

@@ -23,29 +23,38 @@ namespace sycl {
 inline namespace _V1 {
 namespace detail {

+static std::function<void(void *)> CustomDeleterForLibHandle =


This can be a normal function, maybe just declare it as a static member of jit_compiler so you can use it for decltype.

done in b326642

jopperm · 2025-01-07T23:59:51Z

sycl/source/detail/jit_compiler.cpp

 static inline void printPerformanceWarning(const std::string &Message) {
  if (detail::SYCLConfig<detail::SYCL_RT_WARNING_LEVEL>::get() > 0) {
    std::cerr << "WARNING: " << Message << "\n";
  }
 }

-jit_compiler::jit_compiler() {
+jit_compiler::jit_compiler()
+    : LibraryHandle(nullptr, CustomDeleterForLibHandle) {


Is this initialisation necessary?

yes, it is. I should pass custom deleter function to unique_ptr constructor.

jopperm · 2025-01-08T00:03:17Z

sycl/source/detail/jit_compiler.hpp

@@ -98,6 +99,7 @@ class jit_compiler {
  CompileSYCLFuncT CompileSYCLHandle = nullptr;
  ResetConfigFuncT ResetConfigHandle = nullptr;
  AddToConfigFuncT AddToConfigHandle = nullptr;
+  std::unique_ptr<void, std::function<void(void *)>> LibraryHandle;


Suggestion: Introduce a type alias above for this.

I could but I don't think that it is necessary, all types are simple and the structure is clear.
Please let me know if you have any concerns.

Can we simply have this as a void * and handle resource freeing in the destructor? I think this overcomplicates code.

It was done in the first version of this PR and changed to unique_ptr according to @jopperm code-review comment. Not sure what should I do then.

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

KseniyaTikhomirova added 2 commits January 3, 2025 06:05

unload library in jit_compiler

34b8c73

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

fix library unloading in kernel_compiler_opencl

ce8cf49

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

KseniyaTikhomirova temporarily deployed to WindowsCILock January 3, 2025 14:57 — with GitHub Actions Inactive

KseniyaTikhomirova had a problem deploying to WindowsCILock January 3, 2025 15:27 — with GitHub Actions Error

fix bug

16e9af9

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

KseniyaTikhomirova temporarily deployed to WindowsCILock January 3, 2025 15:37 — with GitHub Actions Inactive

KseniyaTikhomirova temporarily deployed to WindowsCILock January 3, 2025 16:07 — with GitHub Actions Inactive

KseniyaTikhomirova changed the title ~~Fix leak in online compiler~~ [SYCL] Unload ocloc in jit_compiler and kernel_compiler_opencl Jan 3, 2025

KseniyaTikhomirova marked this pull request as ready for review January 3, 2025 16:17

KseniyaTikhomirova requested review from a team as code owners January 3, 2025 16:17

KseniyaTikhomirova requested a review from uditagarwal97 January 3, 2025 16:17

uditagarwal97 reviewed Jan 3, 2025

View reviewed changes

uditagarwal97 approved these changes Jan 3, 2025

View reviewed changes

uditagarwal97 mentioned this pull request Jan 4, 2025

Resource leaks in online compiler implementation #16330

Open

jopperm reviewed Jan 6, 2025

View reviewed changes

fix code-review comments

c86ef55

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

KseniyaTikhomirova temporarily deployed to WindowsCILock January 7, 2025 14:08 — with GitHub Actions Inactive

KseniyaTikhomirova changed the title ~~[SYCL] Unload ocloc in jit_compiler and kernel_compiler_opencl~~ [SYCL] Unload libraries in jit_compiler and kernel_compiler_opencl destructors Jan 7, 2025

KseniyaTikhomirova requested a review from jopperm January 7, 2025 14:11

KseniyaTikhomirova temporarily deployed to WindowsCILock January 7, 2025 14:38 — with GitHub Actions Inactive

jopperm reviewed Jan 8, 2025

View reviewed changes

fix code review comments 2

b326642

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

KseniyaTikhomirova temporarily deployed to WindowsCILock January 8, 2025 16:22 — with GitHub Actions Inactive

KseniyaTikhomirova requested review from jopperm and victor-eds January 8, 2025 16:25

KseniyaTikhomirova temporarily deployed to WindowsCILock January 8, 2025 16:52 — with GitHub Actions Inactive

jopperm approved these changes Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Unload libraries in jit_compiler and kernel_compiler_opencl destructors #16517

[SYCL] Unload libraries in jit_compiler and kernel_compiler_opencl destructors #16517

KseniyaTikhomirova commented Jan 3, 2025 •

edited

Loading

uditagarwal97 left a comment

KseniyaTikhomirova commented Jan 3, 2025

jopperm left a comment

jopperm Jan 6, 2025

jopperm Jan 6, 2025

KseniyaTikhomirova Jan 7, 2025

KseniyaTikhomirova Jan 7, 2025

jopperm Jan 7, 2025

KseniyaTikhomirova Jan 8, 2025

jopperm Jan 7, 2025

KseniyaTikhomirova Jan 8, 2025

jopperm Jan 7, 2025

KseniyaTikhomirova Jan 8, 2025

jopperm Jan 7, 2025

KseniyaTikhomirova Jan 8, 2025

jopperm Jan 7, 2025

KseniyaTikhomirova Jan 8, 2025

jopperm Jan 8, 2025

KseniyaTikhomirova Jan 8, 2025

victor-eds Jan 8, 2025

KseniyaTikhomirova Jan 8, 2025

victor-eds Jan 8, 2025

		jit_compiler::~jit_compiler() {}

[SYCL] Unload libraries in jit_compiler and kernel_compiler_opencl destructors #16517

Are you sure you want to change the base?

[SYCL] Unload libraries in jit_compiler and kernel_compiler_opencl destructors #16517

Conversation

KseniyaTikhomirova commented Jan 3, 2025 • edited Loading

uditagarwal97 left a comment

Choose a reason for hiding this comment

KseniyaTikhomirova commented Jan 3, 2025

jopperm 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

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

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

KseniyaTikhomirova commented Jan 3, 2025 •

edited

Loading