refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry#8891
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
9df2529 to
93d4604
Compare
…ue callback registry Isolates the counter-collection slice of the callback-removal effort (reference PRs ROCm#8730 / ROCm#8586) into a small, single-service change, per review guidance to land callback removal one service at a time. Independent of kernel replay. Counter collection no longer registers a queue_callbacks_t with the HSA queue controller. Instead the HSA write interceptor and async signal handler call explicit free functions: - counters::write_hook / signal_completion_hook / is_any_active (new counters/queue_hooks.{hpp,cpp}) iterating active dispatch_counter_collection contexts and calling the existing queue_cb / completed_cb - hsa/queue_hooks/client_ids.hpp: stable inst_pkt producer tags The per-queue callback registry is intentionally retained for thread-trace, pc-sampling and spm; those migrate in follow-up PRs. The callback thread (callback_thread_start/stop) is preserved. The WriteInterceptor gate and should_batch predicate detect counters via is_any_active() (it no longer contributes to queue.get_notifiers()). Removes the now-unused queue_id from counter_callback_info and updates counter tests off iterate_callbacks. Co-authored-by: Cursor <cursoragent@cursor.com>
93d4604 to
ecf8d24
Compare
vlaindic
left a comment
vlaindic
left a comment
There was a problem hiding this comment.
Overall, I'm fine with the PR, added couple optional comments.
Also, please check the AI generated comments.
PR #8891 Review — refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry
Status: "Not ready to Review" label is set — this review is early feedback.
Findings
⚠️ IMPORTANT [hygiene]: Test Plan and Test Result sections are empty
Issues Detected
- Both
## Test Planand## Test Resultcontain only the template placeholder comments — no actual content.
Suggested Actions
- Fill in the Test Plan: at minimum, state which tests were run (counter-tests, rocprofv3 counter integration tests) and on which hardware.
- Fill in the Test Result: paste pass/fail summary. Even "counter-tests: all pass on MI300X" is sufficient.
Risk/Impact Assessment
- Medium — reviewers cannot verify that the change was exercised before merging.
⚠️ IMPORTANT [hygiene]: JIRA ID is indirect and exempt condition requires validation
Issues Detected
- The PR says "This PR does not have a JIRA ticket but is motivated by a ticket like that of kernel replay: AIPROFSDK-68." The JIRA ID is a reference to a related ticket, not the ticket for this work.
- The description does provide a clear, standalone justification (problem, change, validation path via cross-linked PRs), so the exemption partially applies — but the "motivated by a ticket like" phrasing is weaker than needed.
Suggested Actions
- Either create a JIRA ticket for this refactor and link it directly, or explicitly state "No JIRA ticket; standalone PR — see Technical Details for full justification." Don't use "like AIPROFSDK-68" phrasing, which implies an indirect relationship.
Risk/Impact Assessment
- Low — traceability concern only.
⚠️ IMPORTANT [clarity]: Copyright year is 2025 on all newly added files — should be 2026
Issues Detected
All five newly added files carry Copyright (c) 2025:
counters/queue_hooks.cppcounters/queue_hooks.hppcounters/tests/queue_hooks_test.cpphsa/queue_hooks/client_ids.hpp- (CMakeLists.txt has no copyright header — that's fine)
Suggested Actions
- Update copyright header in
counters/queue_hooks.cppfrom 2025 to 2026 — file is newly added in this PR. - Update copyright header in
counters/queue_hooks.hppfrom 2025 to 2026 — file is newly added in this PR. - Update copyright header in
counters/tests/queue_hooks_test.cppfrom 2025 to 2026 — file is newly added in this PR. - Update copyright header in
hsa/queue_hooks/client_ids.hppfrom 2025 to 2026 — file is newly added in this PR.
Risk/Impact Assessment
- Low — policy/compliance issue only.
⚠️ IMPORTANT [generic+clarity]: Attach-mode in-flight dispatch race is deleted without explanation
Issues Detected
The old stop_context contained an explicit guard: when registration::is_attached(), it removed the per-queue callback so new dispatches during the detach drain could pass through without incrementing _active_kernels. That block is deleted in this PR with only the comment:
"No per-queue callback to remove; counters::write_hook no-ops once dispatch_counter_collection is disabled above."
The comment asserts the new path is safe, but the assertion is not obviously correct:
stop_contextsetsenabled = falseunder a write lock.write_hookcallsqueue_cb, which readsenabledunder a read lock — so the no-op is real for new dispatches.- But
get_active_contexts()is a separate snapshot: a context can still appear inget_active_contexts()for some window afterenabledis set to false (the context is only removed from the active set whenrocprofiler_stop_contextfully deactivates it). - During that window,
is_any_active()returnstrue,should_batch_packetsstaysfalse, andwrite_hookis called — butqueue_cbcorrectly short-circuits. This is safe.
However: the PR description does not explain this reasoning, and the inline comment does not mention the window. A reviewer reading this will correctly ask "what about in-flight dispatches during attach-mode detach?" and have no answer in the code.
Suggested Actions
- Add a comment at the deletion site in
stop_contextexplaining the two-phase safety:enabledis set to false first (makingqueue_cbno-op for new dispatches), and the context will be removed fromget_active_contexts()when the stop completes, at which pointis_any_active()returns false andshould_batch_packetscan return to true. - Add a sentence to the PR description's "Technical Details" section covering the attach-mode case and why the old
is_attached()block is no longer needed.
Risk/Impact Assessment
- Medium — correctness argument is sound based on the code, but the missing explanation creates review friction and risks a future author misunderstanding the invariant.
⚠️ IMPORTANT [generic]: write_hook and signal_completion_hook in queue_hooks.cpp lack function docstrings
Issues Detected
The header queue_hooks.hpp has terse one-line comments above write_hook and signal_completion_hook, but the .cpp implementations have no docstrings at all. The functions carry non-trivial contracts:
write_hookmutatesinst_pkt(appends) and OR-foldsis_serialized— callers must understand these are out-params, not inputs.signal_completion_hookiterates all active contexts (not the one that produced the packet) — the reason for this is subtle.
Suggested Actions
- Add a brief comment to
write_hookinqueue_hooks.hppnoting thatinst_pktandis_serializedare output params (appended/OR-folded respectively), and that the function is called once per kernel dispatch fromWriteInterceptor, not once per active context. - Add a comment to
signal_completion_hookexplaining that it re-queries active contexts rather than taking a captured context pointer because the completion fires asynchronously and contexts may have started/stopped since the dispatch.
Risk/Impact Assessment
- Low — maintainability concern, not correctness.
💡 SUGGESTION [generic]: queue_hooks_test.cpp — single negative-path test is thin for a new public hook
Issues Detected
queue_hooks_test.cpp tests only is_any_active() returning false with no active context. The two primary hooks (write_hook, signal_completion_hook) have no unit-level tests beyond what the existing core.cpp integration tests indirectly exercise.
Suggested Actions
- This is acknowledged as a host-only, GPU-free unit test file, which limits what can be tested without standing up HSA. At minimum, extend
queue_hooks_test.cppwith a test that activates a mock context (the existingcore.cpptest infrastructure already does this) and assertsis_any_active()returns true. This validates the full active/inactive cycle without GPU hardware. - Consider extending
counters/tests/core.cpp:start_stop_buffered_ctxto assert thatcounters::is_any_active()is true after start and false after stop — symmetrical to the olditerate_callbackscheck that was removed.
Risk/Impact Assessment
- Low — the existing integration tests cover real behavior; this is about defense-in-depth at the unit level.
💡 SUGGESTION [hygiene]: client_ids.hpp "unused ids reserved" comment could name the follow-up PRs
Issues Detected
client_ids.hpp says:
"Services are migrated off the per-queue callback registry one at a time; the unused ids are reserved so the tag values stay stable across those PRs."
The constants THREAD_TRACE_CLIENT_ID, PC_SAMPLING_CLIENT_ID, SPM_CLIENT_ID are not used by any code in this PR.
Suggested Actions
- Add references to the follow-up PRs (PR #8790 for thread trace is already mentioned in the description) as comments next to each unused constant, or at least note the planned migration order. This prevents a future developer from wondering if these are dead code.
Risk/Impact Assessment
- Low — cosmetic/maintainability only.
Summary
| Severity | Count | Tags |
|---|---|---|
| 4 | hygiene (2), clarity (1), generic+clarity (1) | |
| 💡 SUGGESTION | 2 | generic (1), hygiene (1) |
Overall Risk: Medium — The refactor logic is sound, the primary behaviors (enabled guard, no_real_consumers gate, should_batch disable) are correctly wired. The main concern is the missing test plan, the unexplained attach-mode safety argument, and stale copyright years.
| @@ -0,0 +1,99 @@ | |||
| // MIT License | |||
| // | |||
| // Copyright (c) 2025 Advanced Micro Devices, Inc. All rights reserved. | |||
There was a problem hiding this comment.
| // Copyright (c) 2025 Advanced Micro Devices, Inc. All rights reserved. | |
| // Copyright (c) 2026 Advanced Micro Devices, Inc. All rights reserved. |
| } // namespace | ||
|
|
||
| void | ||
| write_hook(const hsa::Queue& queue, |
There was a problem hiding this comment.
Could we think of a better name? kernel_dispatch_hook or something similar. Or kernel_dispatch_phase_enter_hook or something. Write might be a bit misleading as it's not clear that this is about write interceptor hook.
There was a problem hiding this comment.
I'd agree the name isn't great - I actually think kernel_dispatch_phase_enter_hook would be reasonably appropriate here. I will look into changing it to that.
| constexpr int64_t COUNTERS_CLIENT_ID = 1; | ||
| constexpr int64_t THREAD_TRACE_CLIENT_ID = 2; | ||
| constexpr int64_t PC_SAMPLING_CLIENT_ID = 3; | ||
| constexpr int64_t SPM_CLIENT_ID = 4; |
There was a problem hiding this comment.
Any reason for not using enum? Becuase we need int64_t?
There was a problem hiding this comment.
I don't think we'd ever need an int64_t for a client id - I'd say it's a reasonably safe change to use enum.
4 participants
Motivation
The purpose of this PR is to isolate the counter-collection slice of the callback-removal effort (reference PRs #8730 / #8586) into a small, single-service change, per review guidance to land callback removal one service at a time.
Note the associated focused effort of the PR #8790 for migrating thread trace off the per-queue callback registry.
The kernel replay feature PR #7960 (and #8622) is greatly enhanced by this PR. However, note this is independent of the kernel replay feature and can be used for other features in ROCprofiler-SDK.
Technical Details
Counter collection no longer registers a queue_callbacks_t with the HSA queue controller. Instead the HSA write interceptor and async signal handler call explicit free functions:
The per-queue callback registry is intentionally retained for thread-trace, pc-sampling and spm; those migrate in follow-up PRs. The callback thread (callback_thread_start/stop) is preserved. The WriteInterceptor gate and should_batch predicate detect counters via is_any_active() (it no longer contributes to queue.get_notifiers()). Removes the now-unused queue_id from counter_callback_info and updates counter tests off iterate_callbacks.
Issue Tracking
This PR does not have a JIRA ticket but is motivated by a ticket like that of kernel replay:
AIPROFSDK-68. Please see the kernel replay PR #7960 for understanding the need for this PR. Note that this PR should cleanly merge into develop before and/or without kernel replay feature PR #7960.
Test Plan
Test Result
Submission Checklist