Handle null pointer in set_callback to improve performance
Details
Diff Detail
Event Timeline
Can we have a test for this? Like setting the callback, unsetting it again and checking that the function isn't called?
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.
Can't we shorten this?
I'd prefer
ompt_enabled.event_name = (callback != 0); \
to the if which then assigns one or zero.
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 | Not needed. |
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.
runtime/test/ompt/misc/unset_callback.c | ||
---|---|---|
8 | 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. |
runtime/test/ompt/misc/unset_callback.c | ||
---|---|---|
8 | 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. |
Not needed.