Repository navigation
Added database probe - #13388
Added database probe#13388
Conversation
adanalis-amd
left a comment
There was a problem hiding this comment.
Probe result in the C API
-
ph_database_probe_texposes the status but not the reason. The C++ result carriesmessageanduuid, but C callers (roc-Optiq, per the PR motivation) can only show something like "metadata_malformed", not why.ph_ctx_create()is coarser still:to_ph_result()turns file-not-found, unreadable, malformed and ambiguous into a singlePH_RESULT_INVALID_DATABASE. Consider passing the message through, either as a caller-provided buffer on the probe or with aph_probe_database_message()-style accessor. -
ph_database_probe_thas nostruct_sizeor version field, so addingmessageoruuidlater changes its size and breaks existing callers. Since the struct is new, now is the cheap time to add one, e.g. a leadingsize_t sizethat the caller sets tosizeof(ph_database_probe_t).
| @@ -221,11 +229,6 @@ ph_ctx::read_trace(const stop_requested_fn& stop_requested, const progress_fn& p | |||
| if(stop_requested()) return false; | |||
|
|
|||
| if(progress) progress(0.0, "Reading schema version"); | |||
There was a problem hiding this comment.
The schema version is now read in accept_trace(), so this "Reading schema version" progress step no longer does anything. It can be dropped.
|
I had to add more changes to the PR, because I needed cumulative change. With these changes plus changes in Optiq I can load timeline and histogram. Everything looks fine and works quite fast, thanks to Alexandar. All table and event operations are not yet working. It shouldn't be much efforts adding event operations, like flow and stack, but much more efforts to add table data and searches. |
| double value{}; | ||
| }; | ||
|
|
||
| struct interval_row_t |
There was a problem hiding this comment.
Every row type in this header is named *_result. Rename to interval_result.
| [[nodiscard]] std::vector<interval_row_t> query_intervals( | ||
| const std::string& sql) const | ||
| { | ||
| return m_backend | ||
| ->create_read_statement_executor<interval_row_t>(sql, | ||
| &interval_row_t::event_id, | ||
| &interval_row_t::start_ts, | ||
| &interval_row_t::end_ts)() | ||
| .to_vector(); | ||
| } |
There was a problem hiding this comment.
query_intervals prepares a statement and executes it in the same call, from a raw SQL string built in histogram.cpp. Move the SQL here and create the interval statements (one or more, with ? parameters) in the constructor like the other statements, expose them as statement_func_t, and let the histogram call them to fetch the data.
| ph_optional_uint64_t absolute_index; | ||
| ph_optional_uint64_t logical_index; | ||
| ph_optional_uint64_t uuid; | ||
| uint64_t row_id; /**< rocpd_info_agent.id. */ |
There was a problem hiding this comment.
row_id exposes a rocpd table id through the public API. It exists only because ph_stream_t.pid, ph_queue_t.pid and nid also carry database ids instead of public identifiers. It is specific to this database, so it becomes a problem as soon as another format is added, which has no such row id. Express those relationships with public identifiers (the same applies to ph_process_t.row_id and ph_thread_t.row_id) and drop row_id.
There was a problem hiding this comment.
Ok, I'll do that. I would use ph_agent_t.id, but I think it's not right to take it from static_caststd::uint32_t(agent->type_index). We may have database with CPU and GPU agents in the same table. So we need to take it from agent->absolute_index to make unique identifier.
There was a problem hiding this comment.
Without these IDs we are loosing topology relations. We will need to synthesize row ids and replace them in all information tables. So, yeah, I disagree we are removing them. And that creates event more inconsistency between databases. For example, some databases may have process row id equal to PID, some may just have sequential number. Not talking about table view. For example I get table row as is, with indexes, no strings. Now I want to reach to information table and get a name of corresponding to the index. Without proper row_id I cannot do that. So it's not private ID anymore after you agreed to give us information tables. I'm considering not implementing this change and leave row_id.
| * "3.0" is major 3, minor 0, patch 0. | ||
| */ | ||
| inline std::optional<profiler_hub::version_t> | ||
| parse_schema_version_text(std::string_view text) |
There was a problem hiding this comment.
Parsing "3", "3.0" or "3.0.1" into a version_t is not specific to the schema. Move it to a private version_parser.hpp so any version text reuses it, and keep only the tag resolution (resolve_schema_version) schema-specific, in schema_version.hpp. This also removes the confusing schema_version_text.hpp name.
A rocpd uuid can contain underscores, so taking the text after the last underscore of an arbitrary table looks up string tables that do not exist and the trace fails to open.
Unrecorded agent and queue ids share the zero track without COALESCE. A host thread omits rows that resolve to a device queue or stream, correlation follows the schema 3 reachability graph, and nesting depth reuses a free lower level.
rocpd_event call-stack JSON is usually empty, so a region stack is that region plus the callers still active on its track. Each frame carries the region name and row id.
Motivation
Before client like roc-Optiq uses profiler hub to open database, it needs a tool to check if database is supported by profiler hub.
Technical Details
Database probing rejects an unsupported trace before any reader is opened.
probe_database() and ph_probe_database() open the file read-only and do not create or modify it. The probe checks, in order:
Version tags are read from that metadata table. If schema_version_major, schema_version_minor, and schema_version_patch are all present, those values are used. Older traces that only have schema_version are still accepted: "3" is 3.0.0, "3.0" is 3.0.0, and "3.0.1" is 3.0.1.
ph_ctx_create() runs this probe before the connection pool opens. A rejected file returns one of the new result codes instead of failing later inside a reader:
PH_RESULT_UNSUPPORTED_DATABASE
PH_RESULT_INVALID_DATABASE
PH_RESULT_METADATA_MISSING
PH_RESULT_UNSUPPORTED_SCHEMA_VERSION
Another database format is added by registering a detector. Another schema is added by registering the versions its reader accepts.