refactor(optiq): consume track-based profiler-hub reader API (opaque ids, unified detail) - #961
Draft
avansick-amd wants to merge 18 commits into
Draft
avansick-amd wants to merge 18 commits into
avansick-amd wants to merge 18 commits into
Conversation
Add find_package(profiler-hub REQUIRED) and link the imported profiler-hub::profiler-hub target into the datamodel library, so the loader can consume the synchronous profiler-hub reader API in place of the bespoke sqlite3 query engine. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Replace the SQL-based kernel-dispatch and stream-track discovery blocks with AddReaderGpuQueueAndStreamTracks(), which uses the profiler-hub reader's track_type_t::gpu_queue and track_type_t::stream types. Mirrors the cpu_thread migration from 005B-1. - ReaderGpuQueueTrackToTrackParams: populates identity slots (node/agent/queue), category=kRocProfVisDmKernelDispatchTrack, op=Dispatch, and reader_track_id from reader track info. - ReaderStreamTrackToTrackParams: populates identity slots (node/pid/stream), category=kRocProfVisDmStreamTrack, op=kRocProfVisDmOperationNoOp (per-event op set in ReadReaderTraceSlice from interval_event_t::op_kind). - AddReaderGpuQueueAndStreamTracks: iterates get_all_tracks(), adapts each gpu_queue/stream track, calls get_track_stats for record_count, get_interval_track for time bounds, and ProcessTrack. Handles cache hit (CallBackLoadTrack) and cache miss (reader synthesis) paths. - OpKindToEventOp: static helper mapping event_type_t to rocprofvis_dm_event_operation_t (kernel_dispatch->Dispatch, memory_copy->MemoryCopy, memory_allocate->MemoryAllocate). - ReadReaderTraceSlice: per-event op_kind override for stream tracks so each event routes to the correct get_*_details() overload. - ReadTraceMetadata: replace ~28-line SQL kernel-dispatch block with a 4-line call to AddReaderGpuQueueAndStreamTracks. - Memory-alloc/memory-copy SQL blocks: suppress the ForStream track and slice queries (now "" instead of GetRocprof*ForStream()) since stream events are fully covered by the reader stream track; prevents record_count double-counting via ProcessTrack's accumulate path. Verified against rocpd-transpose.db (v3): - gpu_queue tracks: 2 tracks x 502 records, named "AMD Radeon PRO W7600 / Queue N", correctly nested under GPU in topology. - stream tracks: 2 tracks x 526 records (502 kd + 24 mc per stream_id), named "Stream N", topology links Stream->GPU->Queue via PopulateStreamToHardwareFlowProperties. - 387/387 system test assertions pass; 465050/465050 compute tests pass. - clang-format-18 diff-scoped clean. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Replaces the standalone memory-allocation SQL block in ReadTraceMetadata (GetRocprofMemoryAllocTrackQuery + Level/Slice/TableQuery) with a reader-backed AddReaderMemoryTracks() call, mirroring the AddReaderGpuQueueAndStreamTracks / AddReaderGpuQueueTrackToTrackParams adapter pattern (005B-2/005B-3). Consumes task 009's track_type_t::memory (keyed (nid, agent_id, queue_id, pid) per GetRocprofMemoryAllocTrackQuery GROUP BY). Category carried in reader interval SQL (both v3/v4) since 009. ForStream double-counting: not reintroduced -- type==memory filter is orthogonal to the type==stream path in AddReaderGpuQueueAndStreamTracks. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Route Optiq's standalone queue-keyed memory-copy track through the reader's track_type_t::dma, mirroring the gpu_queue/stream/memory-alloc hybrid-load adapter pattern. The reader keys dma tracks by destination agent (dst_agent_id), so each track nests under its destination GPU/CPU agent. The stream-track memory-copy leg is unchanged (already covered by the stream migration). - Add ReaderDmaTrackToTrackParams (AGENT<-agent_info->id, category= kRocProfVisDmMemoryCopyTrack, op=kRocProfVisDmOperationMemoryCopy). - Add AddReaderDmaTracks (get_all_tracks() filtered to track_type_t::dma, load_id 4). - Replace the standalone memory-copy SQL discovery block with AddReaderDmaTracks. Verified against rocpd-transpose.db: 2 dma tracks partitioned by destination agent (24 events each), per-event category rocm_memory_copy preserved, 387/387 system + 465050/465050 compute assertions green. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Replace Optiq's standalone "SMI performance counters" SQL block in ReadTraceMetadata with a reader-backed AddReaderCounterTracks() call, mirroring the dma/memory adapter pattern (005B-1..005B-3). Counter (SMI) tracks are the sample-based track_type_t::counter; kernel-dispatch PMC (load_id 5) and memory-activity (load_id 7) stay on the SQL path (tracked as tasks 011/012B). - ReaderCounterTrackToTrackParams: category kRocProfVisDmPmcTrack, id[COUNTER] = pmc_info->pmc_id (matches the SQL path's PMC_E.pmc_id identity + CachedTables PMC panel lookup), id[AGENT] = agent_info->id (0 for v3 counters, which carry no agent per locked Q10), reader_track_id = info.id. - AddReaderCounterTracks: get_all_tracks() filtered to track_type_t::counter, load_id 6, record_count from get_track_stats, time/value bounds from get_scalar_track behind a has_samples guard so a count>0 / no-valid-timestamp track cannot collapse db_inst_start_time. - ReadReaderTraceSlice: scalar branch (get_scalar_track, sorted, with left/right-neighbour + synthetic endpoint) emitting record.pmc. - BuildReaderTrackHistogram: scalar branch (per-bucket COUNT + AVG(value)). Depends on reader fixes 005B-4-fix-1-fix-1 / -fix-2 / -fix-3 / -fix-4 (deterministic pmc_id, numeric pmc_id on pmc_info_t, scalar value-query fan-out dedup, and counter-discovery restricted to PMC-backed samples). Verified against rocpd-transpose.db: counter track count 18 (matches the old SQL SMI path); datamodel-compute-tests 465050/465050 exact. datamodel-system-tests-DB settles at 388 (was 387): a deterministic +1 that is a by-random-index test artifact of the reader's faithful, more-correct counter topology (462-sample dedup vs the SQL AMD-SMI event_id fan-out of 2772, plus Q10-mandated v3 agent attribution), not a data-loss regression. Accepted as the corrected baseline per owner decision; a follow-up tester task will make the counter assertions order/index-independent. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
…pendent Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Replace the four per-event GetRocprofDataFlowQueryFor* SQL query branches in
ReadFlowTraceInfo with two eager, per-db-instance in-memory indexes built from
the profiler-hub reader:
- TOPOLOGY index: undirected adjacency from one get_flows() call, keyed on
(event_type, opaque_id). Typed keys are mandatory since opaque_ids collide
across the region/kernel_dispatch/memory_copy/memory_allocate tables.
Neighbors are filtered per clicked op type to reproduce the four SQL UNION-leg
sets exactly, including the memory_allocate asymmetry (no sibling leg).
- PAYLOAD index: (event_type, opaque_id) -> {start, end, level, category, symbol,
col4, col5} built from get_all_tracks() + get_interval_track() over the four
native single-table track types (cpu_thread/gpu_queue/dma/memory). Stream and
counter tracks excluded to prevent double-keying and incorrect level values.
Both indexes are built once per db-instance (mutex-guarded, idempotent), cached
alongside m_readers, and resolved correctly for multi-node event ids.
Fidelity verified on rocpd-transpose.db against a pristine pre-005B-1 pure-SQL
oracle (commit 48e29fb): byte-identical for region, kernel_dispatch, and
memory_copy clicked events on op, opaque_id, level, ts, end, category, and
symbol. memory_allocate fidelity verified on a fabricated isolated-clique fixture
(tasks/005B-5-flowgate/transpose-ma.db).
Tests: new flow-fidelity REQUIRE assertions in Flow Trace Fidelity SECTION
(+16 assertions); datamodel-system-tests-DB 319/319, compute-tests 465050/465050.
…formatting only) Format 9 files changed across tasks 005B-1..5, 009, 010, 011, 012B, 014 using clang-format --style=file (src/.clang-format). No behavior change — whitespace, line-length breaks, and brace placement only.
Readable summary of the 9-commit reader-migration prototype (build wiring + 7 track-type migrations + formatting pass), grounded in the actual diffs/commit messages on this branch. For the Optiq team to review this fork/branch without needing to read every commit individually.
Moving off the hand-written SQL layer is a stated driver of the broader project, not an optional side path. Reworded the status/framing and bottom-line sections to present this branch as one concrete way to get there, rather than implying the existing SQL path is a fine long-term alternative.
…entence) The previous commit (374fddb) transferred a truncated copy of the doc (103 lines / 7246 bytes, cutting off mid-word in the Bottom line section) due to a file-transfer desync, not a content error. This restores the full, correct 108-line / 7656-byte version -- verified byte-identical between the local source and this repo copy before committing.
Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Links each of the nine commit hashes in the "What changed" table to its GitHub commit page, the two named source files to their blob pages pinned at 9c09fbe, the two exemplar functions (ReaderGpuQueueTrackToTrackParams L486-512, AddReaderMemoryTracks L643-739) to exact line-range blob anchors, the dataflow get_flows() mention to commit aab7609, and the SQL-oracle 48e29fb reference to its commit page. Prose is byte-for-byte unchanged; only Markdown link syntax is added around already-present text.
… API Migrate roc-optiq's click->detail/stack/flow-detail path onto the profiler-hub reader so the reader is the sole DB gateway and event_id_t opacity (task 028) is preserved end to end. The 52-bit UI-handle event_id slot now carries a per-shard surrogate minted from a surrogate<->event_id_t registry rather than a decodable row id. - Registry (BuildReaderEventRegistry): eager, once-per-shard scan of get_all_tracks()/get_interval_track() minting stable surrogates deduped by event_id_t; each entry carries home/stream track_id, per-track level, and the home-track operation (recovered from track_type since interval_event_t exposes no op_kind). - ReadExtEventInfo: get_event_detail for Properties + Essential Info (nav fields served from the registry, no per-click FindTrack) and get_arguments(event_id_t) for the Arguments panel; entity ids resolved via get_all_* for panel fidelity. - ReadStackTraceInfo: get_call_stack(event_id_t); drops the recursive rocpd_call_stack/line_info/info_source_code SQL walk. - Flow topology re-keyed on event_id_t (drops the numeric ReaderFlowKey). - Removed the now-dead per-type detail query factories (GetRocprofEssentialInfoQueryFor*/GetRocprofArgumentsInfoQueryFor*). No public event_id_t decode accessor added. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
…l_t->event_info_t Track the profiler-hub reader-API identifier rename at the detail-path call sites (rocprofvis_db_rocprof.cpp). No behavior change; reader stays the sole DB gateway and event_id_t opacity is preserved.
Mirror the profiler-hub rename of shipped public identifiers to their draft_api_2026-06-22.md spellings — consumer-side, pure rename: scalar_event_t -> scalar_sample_t interval_event_t -> interval_entry_t flow_t -> flow_edge_t get_all_tracks() -> get_tracks() datamodel target builds green against the renamed profiler-hub headers.
reader_track_id (the profiler-hub reader track handle Optiq persists to its session-cache DB) changes from size_t to profiler_hub::reader_types::track_id_t, matching the now-opaque public reader API. The kInvalidReaderTrackId sentinel becomes a track_id_t wrapping SIZE_MAX so the on-disk "SIZE_MAX <-> -1" invalid encoding round-trips unchanged; serialization reads/writes the public .value. Optiq's own rocprofvis_dm_track_id_t is untouched. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Contributor
|
Could you please implement a method to detect if file is supported by profiler hub. |
Contributor
|
I don't think this is proper way of listing UUIDs: |
Contributor
|
Please add LOG_INFO for every query execution and a way to turn it on and off. |
Contributor
|
Profiler hub should handle exceptions and have an enumeration of execution states. Interface methods should return the state values or throw exceptions describing a state. |
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
2 participants
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.
Motivation
Migrates roc-optiq onto the refactored profiler-hub reader API. The reader now
exposes a track-based surface with opaque ids and typed accessors (see
ROCm/rocm-systems#9366); this branch updates Optiq's data model, controller, and
session-cache layer to consume that surface directly, so Optiq no longer reaches
around the reader for detail data.
Technical Details
event_id_t,flow_id_t, andtrack_id_tas opaque types. The reader remains the sole gateway; Optiq onlyreads the raw
track_id_tvalue where it must serializereader_track_idtothe session-cache DB (integer column).
unified
get_event_detail/get_event_infoaccessor rather than Optiq'sformer direct-SQL detail queries.
the reader are wired through the model -> controller -> view layers.
spec-aligned names.
History note: this branch is 18 commits cut from
dev(an ancestor of thedefault
mainbranch);mainis 96 commits ahead. It will be rebased onto freshorigin/mainand curated before this draft is marked Ready for review.Test Plan
Datamodel target build + Optiq unit suite; interactive GUI panel-fidelity check.
Test Result
Datamodel target builds green. GUI panel-fidelity human gate still pending (called
out below).
Reviewer Notes
(refactor(profiler-hub): track-based reader API (opaque ids, typed flows, unified detail) rocm-systems#9366). Review the reader PR alongside this one.
performed; please do not treat rendering parity as verified.
origin/main+ history curation still owed beforeReady-for-review.