Rewrite kernel profiler for correct tree nesting and GPU timing - #3694
Draft
Alexandr-Solovev wants to merge 1 commit into
Draft
Rewrite kernel profiler for correct tree nesting and GPU timing#3694Alexandr-Solovev wants to merge 1 commit into
Alexandr-Solovev wants to merge 1 commit into
Conversation
Rebuild the profiler tree from per-thread stacks tracked in the singleton instead of a global level counter. Regular tasks now record a parent index at push time (the caller thread's innermost open task, with fallback to the innermost regular task across threads for worker-thread threading tasks); level derives from parent + 1. This fixes the case where a task opened on one thread and closed on another (or threading tasks under a regular parent) produced garbled nesting. Merge phase now keys sibling collapse on (parent_idx, name) via an unordered_map, so it is O(n) and correctly matches only true siblings. Levels-and-names on their own were ambiguous: two tasks at the same depth under different parents could collapse into one. Cache ONEDAL_VERBOSE once per process in a function-local static instead of a namespace-scope `volatile int`. The previous form gave every TU that included the header its own copy, so `daal_verbose_val` observed different values across .so boundaries. Move the once-per-process suppression sets for the threading logger and tracer messages onto the profiler singleton so their lifetimes are bounded to the profiler, not the process; hoist unique-name checks under the existing mutex. On the oneAPI side, replace the `start_task(name, queue)` pattern with a `queue_sync_guard` RAII wrapper. The guard issues `wait_and_throw()` before `start_task()` and again on scope exit so recorded durations reflect device-side execution rather than host enqueue. The pre-region wait happens before the timer, so already-in-flight work on the queue is not charged to the region. When the profiler is disabled the guard is a nullptr no-op and no waits are issued. Uniquify the local variable name via __LINE__ so multiple profiled scopes in the same block coexist; drop the ternary-of-mixed-types pattern and gate each START_TASK* macro on the enable predicate directly. SERVICE task variants gate their logger side-effects on `is_service_debug_enabled` (ONEDAL_VERBOSE=5) rather than `is_logger_enabled` (=1/4), matching the pre-refactor behavior — otherwise hot paths like `table2ndarray` would flood the output under LOGGER mode. Validated: make onedal_dpc -j112 clean; CPU + GPU cov_dense_batch and kmeans_lloyd_dense_batch under ONEDAL_VERBOSE=3 produce well-formed analyzer trees with correct parent-child nesting and durations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| } | ||
|
|
||
| // Reads ONEDAL_VERBOSE once per process; the C++17 function-local static gives thread-safe | ||
| // single initialization and a single storage instance across all TUs (fixes the previous |
Contributor
There was a problem hiding this comment.
It could explain what a TU is.
|
|
||
| #include <chrono> | ||
| #include <thread> | ||
| #include <cerrno> |
Contributor
There was a problem hiding this comment.
Are macros from this header used somewhere?
|
|
||
| ~queue_sync_guard() { | ||
| if (queue_) { | ||
| #if (!defined(DAAL_NOTHROW_EXCEPTIONS)) |
Contributor
There was a problem hiding this comment.
Throwing on a destructor is undefined behavior. It should be catched regardless of this macro.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebuild the profiler tree from per-thread stacks tracked in the singleton instead of a global level counter. Regular tasks now record a parent index at push time (the caller thread's innermost open task, with fallback to the innermost regular task across threads for worker-thread threading tasks); level derives from parent + 1. This fixes the case where a task opened on one thread and closed on another (or threading tasks under a regular parent) produced garbled nesting.
Merge phase now keys sibling collapse on (parent_idx, name) via an unordered_map, so it is O(n) and correctly matches only true siblings. Levels-and-names on their own were ambiguous: two tasks at the same depth under different parents could collapse into one.
Cache ONEDAL_VERBOSE once per process in a function-local static instead of a namespace-scope
volatile int. The previous form gave every TU that included the header its own copy, sodaal_verbose_valobserved different values across .so boundaries.Move the once-per-process suppression sets for the threading logger and tracer messages onto the profiler singleton so their lifetimes are bounded to the profiler, not the process; hoist unique-name checks under the existing mutex.
On the oneAPI side, replace the
start_task(name, queue)pattern with aqueue_sync_guardRAII wrapper. The guard issueswait_and_throw()beforestart_task()and again on scope exit so recorded durations reflect device-side execution rather than host enqueue. The pre-region wait happens before the timer, so already-in-flight work on the queue is not charged to the region. When the profiler is disabled the guard is a nullptr no-op and no waits are issued.Uniquify the local variable name via LINE so multiple profiled scopes in the same block coexist; drop the ternary-of-mixed-types pattern and gate each START_TASK* macro on the enable predicate directly. SERVICE task variants gate their logger side-effects on
is_service_debug_enabled(ONEDAL_VERBOSE=5) rather thanis_logger_enabled(=1/4), matching the pre-refactor behavior — otherwise hot paths liketable2ndarraywould flood the output under LOGGER mode.Validated: make onedal_dpc -j112 clean; CPU + GPU cov_dense_batch and kmeans_lloyd_dense_batch under ONEDAL_VERBOSE=3 produce well-formed analyzer trees with correct parent-child nesting and durations.
Description
Checklist:
Completeness and readability
Testing
Performance