[SYCL] Clear extensions functions cache upon context release #5282

s-kanaev · 2022-01-11T13:47:57Z

This is to eliminate reuse of invalid cached values
after context being released.

This is to eliminate reuse of invalid cached values after context being released. Signed-off-by: Sergey Kanaev <[email protected]>

Signed-off-by: Sergey Kanaev <[email protected]>

s-kanaev · 2022-01-11T15:03:40Z

/summary:run

The test is going to be disabled until intel/llvm#5282 is merged Signed-off-by: Sergey Kanaev <[email protected]>

Signed-off-by: Sergey Kanaev <[email protected]>

s-kanaev · 2022-01-12T07:28:41Z

/summary:run

The test is going to be disabled until intel/llvm#5282 is merged Signed-off-by: Sergey Kanaev <[email protected]>

sycl/plugins/opencl/pi_opencl.cpp

Signed-off-by: Sergey Kanaev <[email protected]>

…on-context-release

steffenlarsen

New approach LGTM. Just have some minor suggestions.

sycl/plugins/opencl/pi_opencl.cpp

sycl/plugins/opencl/ext_functions.inc

Co-authored-by: Steffen Larsen <[email protected]>

Signed-off-by: Sergey Kanaev <[email protected]>

s-kanaev · 2022-01-14T08:53:48Z

/summary:run

steffenlarsen

Good stuff! Have a 👍 .

s-kanaev · 2022-01-17T07:37:57Z

The failure on HIP AMD isn't relevant to this patch and is resolved by XFAIL-ing the tests int intel/llvm-test-suite#740

bader

I've made a few style comments as I'm not an expert in this code and can't review design solution.
I'll let @romanovvlad to follow-up with that PR.

bader · 2022-01-17T08:12:35Z

sycl/plugins/opencl/pi_opencl.cpp

+#undef _EXT_FUNCTION_COMMON
+} // namespace detail
+
+struct ExtFuncsCachesT {


Why plural form?

Suggested change

struct ExtFuncsCachesT {

struct ExtFuncsCacheT {

It's plural because each context has it's own cache.

I think it would be reasonable to call the whole map a cache instead of ExtFuncsPerContextT instances.

sycl/plugins/opencl/pi_opencl.cpp

bader · 2022-01-17T08:16:30Z

sycl/plugins/opencl/pi_opencl.cpp

@@ -1397,6 +1480,8 @@ pi_result piPluginInit(pi_plugin *PluginInit) {
  // PI interface supports higher version or the same version.
  strncpy(PluginInit->PluginVersion, SupportedVersion, 4);

+  ExtFuncsCaches = new ExtFuncsCachesT;


Should we use a smart pointer? https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r3-a-raw-pointer-a-t-is-non-owning

It might be an option if the following drawback is resolved. Smart pointer (both shared_ptr and unique_ptr) are not trivially destructible. Having a smart pointer here may lead to potential mem leak or anything else related to order of destructors calls.

Probably, @alexbatashev has smth to comment here if I'm wrong.

Having a smart pointer here may lead to potential mem leak or anything else related to order of destructors calls.

Could you clarify the scenario when "mem leak or anything else related to order of destructors calls" happen and how using a raw pointer solves the problems in such scenarios?

From POV, you can always call release method when you call delete for a raw pointer and have additional mem leak protection for the cases when delete is not called.

The mem-leak and destructor ordering I refer here are not related to the owned object, i.e. cache, but to the smart pointer itself. shared_ptr has two atomic counters. My blind guess is they're in heap. Both unique_ptr and shared_ptr have a 'complex' d-tor which has to check if the pointer owns the object.

Sure thing. When smart pointer is used I'll put call to release method in piTearDown. Bearing this in mind, is there any real need for smart pointer as I won't use its features?

If we change the type of ExtFuncsCaches to a unique_ptr, we may have a problem since unique_ptr can be destructed(as a regular global var) earlier than the latest call to an API which uses this var.

How is that possible? Isn't calling functions from unloaded sharing library is UB?

How is that possible? Isn't calling functions from unloaded sharing library is UB?

A global object can be destroyed earlier then a library is unloaded.

Right, so I expected that plug-in has a single global object managing the lifetime of plug-in data ("plug-in context") and after this global object is destroyed, there can't be any "calls to an API which uses this var".

there can't be any "calls to an API which uses this var".

How this can be guaranteed?

s-kanaev · 2022-01-17T08:38:00Z

/verify with intel/llvm-test-suite#745

sycl/plugins/opencl/pi_opencl.cpp

romanovvlad · 2022-01-17T10:10:34Z

sycl/plugins/opencl/pi_opencl.cpp

    // if cached that extension is not available return nullptr and
    // PI_INVALID_VALUE
-    *fptr = F;
-    return F ? PI_SUCCESS : PI_INVALID_VALUE;
+    *fptr = FuncInitialized.first;


Not sure we get a lot from having lazy initialization for these function pointers. Suggest moving all the initialization to piContextCreate. This should simplify the logic.

Per what was discussed with @steffenlarsen , the main drawback of constant (as opposed to lazy) init in piContextCreate is that use-cases that create lots of contexts without using extensions will be impacted on performance. I reckon it's only synthetic cases, but still...

I'm OK with leaving as is. But still think that if an application creates lots of context this initialization will be that last thing we should worry about. Also this would make "common" case faster and code more simpler because less mutexes and checks should be needed.

romanovvlad · 2022-01-17T10:19:33Z

sycl/plugins/opencl/pi_opencl.cpp

@@ -1397,6 +1480,8 @@ pi_result piPluginInit(pi_plugin *PluginInit) {
  // PI interface supports higher version or the same version.
  strncpy(PluginInit->PluginVersion, SupportedVersion, 4);

+  ExtFuncsCaches = new ExtFuncsCachesT;


If we change the type of ExtFuncsCaches to a unique_ptr, we may have a problem since unique_ptr can be destructed(as a regular global var) earlier than the latest call to an API which uses this var.

…on-context-release

Signed-off-by: Sergey Kanaev <[email protected]>

s-kanaev · 2022-01-17T14:26:20Z

/verify with intel/llvm-test-suite#745

…5282)" (#5433) This reverts commit 2eed402. Revert is due massive performance regression introduced. Employing of spin-lock and/or pre-initialization didn't help reduce performance impact.

The test is going to be disabled until intel#5282 is merged Signed-off-by: Sergey Kanaev <[email protected]>

[SYCL] Clear extensions functions cache upon context release

eeff988

This is to eliminate reuse of invalid cached values after context being released. Signed-off-by: Sergey Kanaev <[email protected]>

s-kanaev requested a review from a team as a code owner January 11, 2022 13:47

s-kanaev requested a review from steffenlarsen January 11, 2022 13:47

s-kanaev force-pushed the clear-ext-func-cache-on-context-release branch 2 times, most recently from c7d36c3 to 68811ba Compare January 11, 2022 14:18

s-kanaev marked this pull request as draft January 11, 2022 14:42

Stylistic change

6878a50

Signed-off-by: Sergey Kanaev <[email protected]>

s-kanaev force-pushed the clear-ext-func-cache-on-context-release branch from 68811ba to 6878a50 Compare January 11, 2022 14:57

s-kanaev marked this pull request as ready for review January 11, 2022 15:03

s-kanaev pushed a commit to s-kanaev/llvm-test-suite that referenced this pull request Jan 11, 2022

[SYCL] Temporary disable get_backend test

02bb0d0

The test is going to be disabled until intel/llvm#5282 is merged Signed-off-by: Sergey Kanaev <[email protected]>

Add new symbol to ABI test

db047b8

Signed-off-by: Sergey Kanaev <[email protected]>

bader pushed a commit to intel/llvm-test-suite that referenced this pull request Jan 12, 2022

[SYCL] Temporary disable get_backend test (#725)

72ad1bb

The test is going to be disabled until intel/llvm#5282 is merged Signed-off-by: Sergey Kanaev <[email protected]>

steffenlarsen reviewed Jan 12, 2022

View reviewed changes

sycl/plugins/opencl/pi_opencl.cpp Outdated Show resolved Hide resolved

sycl/plugins/opencl/pi_opencl.cpp Outdated Show resolved Hide resolved

Sergey Kanaev added 3 commits January 13, 2022 17:27

Change structure of cache to reduce number of locks

65afd32

Signed-off-by: Sergey Kanaev <[email protected]>

Stylistic change

d212fe7

Signed-off-by: Sergey Kanaev <[email protected]>

Merge remote-tracking branch 'public/sycl' into clear-ext-func-cache-…

4320075

…on-context-release

s-kanaev requested a review from steffenlarsen January 13, 2022 14:52

steffenlarsen reviewed Jan 13, 2022

View reviewed changes

sycl/plugins/opencl/pi_opencl.cpp Outdated Show resolved Hide resolved

sycl/plugins/opencl/pi_opencl.cpp Outdated Show resolved Hide resolved

sycl/plugins/opencl/ext_functions.inc Outdated Show resolved Hide resolved

sycl/plugins/opencl/ext_functions.inc Outdated Show resolved Hide resolved

sergei and others added 2 commits January 13, 2022 22:55

Apply suggestions from code review

a020c33

Co-authored-by: Steffen Larsen <[email protected]>

Address review comments

52a6ad5

Signed-off-by: Sergey Kanaev <[email protected]>

s-kanaev requested a review from steffenlarsen January 14, 2022 08:53

steffenlarsen previously approved these changes Jan 14, 2022

View reviewed changes

s-kanaev requested review from romanovvlad and bader January 17, 2022 07:38

bader reviewed Jan 17, 2022

View reviewed changes

s-kanaev mentioned this pull request Jan 17, 2022

[SYCL] Re-enable tests intel/llvm-test-suite#745

Closed

romanovvlad reviewed Jan 17, 2022

View reviewed changes

Sergey Kanaev added 2 commits January 17, 2022 16:02

Merge remote-tracking branch 'public/sycl' into clear-ext-func-cache-…

84502a7

…on-context-release

Add assertion

2a1b3ac

Signed-off-by: Sergey Kanaev <[email protected]>

s-kanaev dismissed steffenlarsen’s stale review via 2a1b3ac January 17, 2022 14:25

steffenlarsen approved these changes Jan 17, 2022

View reviewed changes

s-kanaev requested review from romanovvlad and bader January 18, 2022 07:36

bader removed their request for review January 18, 2022 08:12

romanovvlad merged commit 2eed402 into intel:sycl Jan 18, 2022

sergey-semenov mentioned this pull request Jan 18, 2022

[SYCL] Add an OpenCL function caching issue workaround to get_backend intel/llvm-test-suite#693

Closed

aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023

[SYCL] Temporary disable get_backend test (intel/llvm-test-suite#725)

4969ed9

The test is going to be disabled until intel#5282 is merged Signed-off-by: Sergey Kanaev <[email protected]>

[SYCL] Clear extensions functions cache upon context release #5282

[SYCL] Clear extensions functions cache upon context release #5282

Uh oh!

Conversation

s-kanaev commented Jan 11, 2022

Uh oh!

s-kanaev commented Jan 11, 2022

Uh oh!

s-kanaev commented Jan 12, 2022

Uh oh!

Uh oh!

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-kanaev commented Jan 14, 2022

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

s-kanaev commented Jan 17, 2022

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-kanaev commented Jan 17, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-kanaev commented Jan 17, 2022

Uh oh!

Uh oh!