[OMPT] Handle null pointer in set_callback to improve performance

[sconvent]

Summary

Handle null pointer in set_callback to improve performance

[Hahnfeld]

Can we have a test for this? Like setting the callback, unsetting it again and checking that the function isn't called?

[protze.joachim]

This change is more a performace optimization. (reset the bit in the bitmap to 0)
The callback was set to NULL before and all callback invocation checks for the NULL-pointer.

In some cases we do not enter a code path if a specific callback is not set. With this patch this code path is also not entered if the callback was set and reset to NULL.

I agree that we might add a testcase, but the testcase will not show different behavior for this patch.

[sconvent]

Added testcase

[jcownie]

Can't we shorten this?
I'd prefer

     
ompt_enabled.event_name = (callback != 0);                                           \

to the if which then assigns one or zero.

[sconvent]

Implemented suggestion

[Hahnfeld]

In D41171#953941, @protze.joachim wrote:

This change is more a performace optimization. (reset the bit in the bitmap to 0)
The callback was set to NULL before and all callback invocation checks for the NULL-pointer.

In some cases we do not enter a code path if a specific callback is not set. With this patch this code path is also not entered if the callback was set and reset to NULL.

Then title and summary should say that this is a for improving performace, that's impossible to guess from the changes.

Also, is this test case actually valid? I remember that some callbacks must always be set as pairs, not sure if that's true for parallel though.

runtime/test/ompt/misc/unset_callback.c

8 ↗

(On Diff #126921)

Not needed.

[sconvent]

Removed unnecessary definition.

The parallel_begin and parallel_end callbacks can be set independently by my understanding of the spec. So the testcase should be valid.

[Hahnfeld]

LGTM

[protze.joachim]

runtime/test/ompt/misc/unset_callback.c

8 ↗

(On Diff #126921)

Some OpenMP activity is needed to initialize the OpenMP runtime, let the runtime call ompt_start_tool and ompt_inittialize, so that ompt_set_callback is set to non-NULL.

[Hahnfeld]

runtime/test/ompt/misc/unset_callback.c

8 ↗

(On Diff #126921)

I think Phabricator mangled my comment: I originally meant the unused variable x that Simon removed meanwhile. I agree that we need the first parallel region.

runtime/src/ompt-general.cpp

 OMPT_API_ROUTINE int ompt_set_callback(ompt_callbacks_t which,
                                        ompt_callback_t callback) {
   switch (which) {
 
 #define ompt_event_macro(event_name, callback_type, event_id)                  \
   case event_name:                                                             \
     if (ompt_event_implementation_status(event_name)) {                        \
       ompt_callbacks.ompt_callback(event_name) = (callback_type)callback;      \
-      ompt_enabled.event_name = 1;                                             \
+      ompt_enabled.event_name = (callback != 0);                               \
     }                                                                          \
-    return ompt_event_implementation_status(event_name);
+    if (callback)                                                              \
+      return ompt_event_implementation_status(event_name);                     \
+    else                                                                       \
+      return ompt_set_always;
 
     FOREACH_OMPT_EVENT(ompt_event_macro)
 
 #undef ompt_event_macro
 
   default:
     return ompt_set_error;
   }