Skip to content

Conversation

jzc
Copy link
Contributor

@jzc jzc commented Nov 20, 2023

In #11751, ref counting of kernels objects was changed to be more accurate in order to allow for in-memory caching to be disabled. When getting a kernel form the cache, the ref count the kernel handle is now incremented (when caching is enabled). Thus, a method like ProgramManager::getOrCreateKernel will increment the ref count of the kernel it gets. However, in enqueueImpKernel, when enqueuing a kernel with a kernel bundle,ProgramManager::getOrCreateKernel is called twice, first indirectly by:

kernel SyclKernel =
KernelBundleImplPtr->get_kernel(KernelID, KernelBundleImplPtr);

and second directly by:

// When caching is enabled, kernel objects can be shared,
// so we need to retrieve the mutex associated to it via
// getOrCreateKernel
if (SYCLConfig<SYCL_CACHE_IN_MEM>::get()) {
auto [CachedKernel, CachedKernelMutex, CachedEliminatedArgMask] =
detail::ProgramManager::getInstance().getOrCreateKernel(
KernelBundleImplPtr->get_context(), KernelName,
/*PropList=*/{}, Program);
assert(CachedKernel == Kernel);
assert(CachedEliminatedArgMask == EliminatedArgMask);
KernelMutex = CachedKernelMutex;

This means that the ref count of the acquired kernel is incremented twice, yet the rest of the function will only free once, which leads to a leak of the kernel. As the second comment and asserts say, the only need for the second call to getOrCreateKernel is to fetch the mutex associated to the cached kernel retrieved from the first call, so this PR adjusts get_kernel to save this mutex and forgo this extra getOrCreateKernel call and unintentional additional ref count.

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me! Should we have a note on getOrCreateKernel mentioning that it increments the reference count of the kernel?

@jzc
Copy link
Contributor Author

jzc commented Nov 27, 2023

@steffenlarsen Good idea, I added a comment about the ref count

@AlexeySachkov
Copy link
Contributor

int-convert.cpp is a known issue on Windows, which was reported in #12011, I will proceed with merge for this PR

@AlexeySachkov AlexeySachkov merged commit a1edb96 into intel:sycl Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants