Conversation
The launcher joined arguments into strings that the controller re-split on whitespace, so any path or argument containing a space was torn into several, and the command preview re-derived its own tail and could show something other than what ran. - Backends emit the complete argument list from FlattenToExecution; BuildArgv synthesizes nothing, and target_executable / output_directory become metadata that never reach the command line. Drops set_target_args / set_profiler_args from the C API. - Add SplitArguments so a quoted argument stays one argument, and group the seven positional launch strings into ProfilerLaunchSpec. - Support a working directory, applied to the child only and failing the run rather than writing output somewhere else. - Fix the output getter dropping the last character; name the child's pre-exec exit statuses (126/127/128+n). - Prefer the last labelled ".db" path when scraping the trace location, since profiling Optiq with Optiq interleaves the child's own logs. - Add controller profiler tests for argv composition, quoting, env, and working-directory behavior.
- Name tools with rocprofvis_profiler_tool_t; the controller maps the enum to a binary name and resolves it to a validated absolute path before exec, so no caller-supplied string can become argv[0]. A missing tool is reported as such instead of surfacing as exit code 127. - Delete ProfilerSettings::profiler_path. A ROCm install in a non-standard location is handled by LaunchConfig::tool_directory, a directory whose filename still comes from the tool table. - Resolve remote launches on the remote host; local resolution is never applied to them, in the UI preview or at launch. - Carry the enum in ToolOption and LaunchConfig::tool, replacing the per-backend tool id string and the launcher's separate tool index that could disagree with it. - Add controller tests for resolution order, tool-directory strictness, and an unresolvable tool failing the launch without spawning.
…ackend' into tkarczew/profiler_backend
- Add ProfilerConfig::ValidateWorkingDirectory() to confirm the configured working directory exists locally, so a bad path is reported as an invalid argument from the launching call instead of surfacing as the child's exit code 126 after a failed chdir.
rocprof-sys-instrument treats --output as a rewrite filename and rejects --preset/--trace, so FlattenToExecution no longer applies the run-shaped command to every tool.
- Add ProfilerScrapeEngine, a per-line regex scrape over profiler stdout/stderr that reads artifact paths and progress out of tool output. Patterns are compiled once before anything is spawned, matched against complete lines rather than the growing buffer, applied after ANSI stripping, and skipped on lines over 4 KiB, which is what keeps std::regex's per-platform pathological behaviour out of reach over a multi-minute capture.
- Add ProfilerScrapeRules, a table of those patterns keyed by tool, operation, and tool version, rather than accepting patterns from the caller. How to read `rocprof-compute analyze` output is a property of rocprof-compute and not of whoever launched it, so a caller supplying the pattern would have to be revised in lockstep with a tool it does not own, and every client would carry its own copy of the same regex. This is the same trade as selecting the tool by enum instead of a binary path, and it also means a pattern can never arrive from a preset or a project file.
- Select the highest table entry at or below the stage's version, falling back to the base entry when the version is absent or unparsable, so an unrecognised --version banner degrades to the widest patterns instead of failing a launch and a tool that changes its wording gets a new row rather than an edited one.
- Have each entry declare the key naming its artifact, so a caller does not have to repeat a key name the controller owns; setting the artifact key becomes an override rather than a requirement.
- Key scrape slots by (stage, key) rather than by key, because the rules now come from the tool: a pipeline that runs one tool twice, two capture runs feeding one analyze, legitimately produces the same key in two stages, and each is a separate value that {stageN.key} can name. A lookup by key alone answers from the last stage declaring it, which is what "the pipeline's artifact" means.
- Add ProfilerStage as the authoring handle those settings live on, resolve_stage_placeholders for {stageN.key} substitution in argv, the kRPVProfilerStage object type, and kRocProfVisResultNotAvailable for a value that could not be determined. Match policy stays a C++ enum inside the controller rather than becoming an ABI one, since it is a property of a rule and rules have no ABI surface.
- Cover the engine and the table in roc-optiq-controller-profiler-tests, including compiling every pattern in the table and checking each declared capture group exists, so a typo in a shipped pattern is a test failure rather than something only a user running that tool would hit.
- Leave rocprofvis_controller_profiler_pipeline_tests.cpp out of the build: it drives the stage C ABI, which does not exist until the pipeline loop in ProfilerProcessController lands.
Feed only split on '\n', so a target that redraws progress in place buffered one line for the length of the run and then lost it to the byte cap. '\r' now ends a line too, with a trailing one held back so a CRLF split across reads stays one line, and an over-cap line is discarded rather than accumulated. A skipped line is reported once per stage, and only when a key also went unmatched, since long lines are routine on their own. GetStatus returned Unmatched for a key no stage declares, so a mistyped placeholder read as "the profiler did not report it". Both status getters now return a result code and leave the caller's status alone for an unknown key.
ProfilerProcessController gains a stage vector and a current index, and a fresh executor per stage. UpdateState advances on a zero exit, ending that stage's scrape and relocating its artifact; a non-zero exit records the failing stage and skips the rest. All stage tools resolve before stage 0, so a misconfigured analyze stage fails the launch rather than surfacing after a long capture. A config with no stages is wrapped as one stage with no banner, so today's flat path keeps its console byte-for-byte and there is one execution path to maintain. Remote shares the bookkeeping but stays single-stage.
Cancel marked slots skipped and returned, but UpdateState no longer finalises a run that is not Running, so EndStage never ran: a scraped relative path stayed relative and the last unterminated line was never matched. The drain after the executor stops then fed the engine again and could resolve a slot just reported as abandoned. Cancel now drains and ends the stage itself, and the engine ignores anything fed outside a BeginStage/EndStage pair. A cancelled stage keeps what it scraped - a capture killed part way through has still written the file it named - and only the stages that never start are skipped. The kill happens with m_mutex released. The executor gives the child a SIGTERM grace period and can then block in waitpid, which a process parked in a driver call does not leave promptly, and GetOutput takes the same lock. A flag set under the lock makes Cancel the sole owner of the ending, so no stage is settled twice and a boundary already in flight cannot start the next one. GetExitCode is written when a stage ends and read from the UI thread through the C ABI, so it takes the lock now; Cancel sets it too, rather than leaving a cancelled second stage reporting the first one's code. rocprofvis_profiler_cancel returns the controller's result instead of always reporting success, letting the orchestrator tell "stopped it" from "the run finished between the click and the call", which showed as a spurious "Failed to cancel profiler". On Windows ReadOutput did one 4 KiB read per call, so bytes still in the pipe when the child exited were never read and a chatty tool's console could truncate; it drains until the pipe is empty now. Each executor also kept a second unbounded copy of the whole run's output that nothing read. The rocprof-sys trace rule splits Database: and File: into separate rows, so rule order expresses the preference order the View's ParseTraceOutputPath documents and the two readers of one log cannot pick different files.
kill() failing is not always "already gone": EPERM leaves a live child that the old path forgot. Cancel() now returns a three-way outcome; a refusal ends the run as Failed and says so in the console, instead of sitting at Running until the process dies. The teardown wait no longer keys on "is a process alive", so a local child we could not kill does not hold the session open.
tomk-amd
marked this pull request as ready for review
September 9, 2026 20:53
tomk-amd
requested review from
a team,
Sajeeth-Wimalasuriyan,
amokiche-amd,
drchen-amd and
vstempen
as code owners
September 9, 2026 20:53
drchen-amd
reviewed
Sep 14, 2026
m_cancel_requested carried two meanings: "Cancel owns the ending, so UpdateState must not finalise" and "the user has asked to stop, so no later stage may start". The branch where the child turns out to have already exited is exactly where those diverge - the ending does go back to UpdateState, so the run reports what the child really did rather than a cancellation, but clearing the flag to arrange that also re-enabled the advance. A stage that finished inside the cancel window would then have FinishStageLocked start the next one, running work the user had just asked to abandon, which is what the flag's own comment said could not happen. The two questions are separate fields now, and FinishStageLocked checks the second between "was that the last stage" and the advance, so a single-stage run still reports Completed - nothing was cut short - while a pipeline settles as Cancelled and keeps what the finished stage earned. That same branch returned UnknownError, and the orchestrator reports anything other than Success or NotSupported as "Failed to cancel profiler", so cancelling a run that had just finished normally showed a failure. It is an ordinary race on Windows rather than a corner case: TerminateProcess fails on a child that has already exited, the GetExitCodeProcess fallback confirms it, and kNotRunning comes back. NotSupported is what the not-running early-out already returns, and it is the code the View reads as "the run finished between the click and the call". A stage that fails to start leaves the previous stage's exit code in hand, so a run that ended as Failed could report 0 and read as a success - the hazard EXIT_CODE_NO_STATUS exists to prevent. StartStageLocked resets it on entry, which covers every early return in it as well. PreparePipeline resets it too, for a different window: it was only ever initialised in the constructor, so relaunching a controller reported the previous run's code until the new run's first stage ended. The Windows cancel gave the child exit code 1, indistinguishable from a tool that genuinely failed with 1. POSIX can report a true 128+SIGKILL because a signal was really delivered; there is no equivalent here, so it passes the no-status sentinel instead. It also returned as soon as termination was requested, not once it had happened, handing back a process that still held its output files open - a sharing violation for whoever opened them next. It waits on the handle now, bounded, which is the promise the POSIX path already keeps by always reaping. Two smaller things in the same paths: Cancel indexed m_stages and m_stage_states directly on one line and guarded the same index on the next, so the guard was either dead or too late; both vectors are sized together in PreparePipeline and m_current_stage only ever comes from a stage that started, and a comment now says so. Tool-resolution failure set m_failing_stage without moving that stage off Idle, so GetStageState reported the stage GetFailingStage names as not yet started. The cancel tests are POSIX-only since they need a shell that scripts the same everywhere, so the Windows rewrite is reasoned about rather than exercised.
drchen-amd
reviewed
Sep 16, 2026
GetOutput drains the child's pipe and returns everything the run has printed so far, and ExecuteJob called it three times purely for the drain - once per 100 ms tick, once per 50 ms while teardown is pending, and once at the end - throwing the string away each time. The copy is made under m_mutex, and m_output_text keeps every byte for the life of the run, so the cost grows with the run rather than with what just arrived: on a workload whose own stdout shares this stream, the monitor thread copies a buffer that is already megabytes, ten times a second, while GetState, GetExitCode and Cancel's first critical section wait behind it. PumpOutput does the drain without handing anything back, which is all the loop ever needed - it polls to keep the pipe from filling and stalling the child, not to read what is in it. The two costs underneath this are older than the pipeline and are left for a round of their own, recorded on DrainExecutorLocked so the next person to look does not have to rediscover them. ReadOutput empties the pipe rather than reading a fixed amount, so the time the lock is held is set by how fast the child writes; draining until empty is not optional, because the caller ends the stage as soon as the process is gone and anything still in the pipe at that point is never read at all, so a per-call byte cap has to exempt the last drain of a stage. And m_output_text has no high-water mark: what a long run should discard, and when, is a question the profiler console has not had to answer yet.
drchen-amd
approved these changes
Sep 16, 2026
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
Continue improving the profiling backend. This PR adds additional pieces needed for running multiple tools.
No attention given to UI unless necessary for functionality. (no aesthetic or layout fixes and improvements).
Technical Details
AI generated overview:
Profiler: scrape tool output, and run a config's stages as a sequence
Phase 1 of the profiler pipeline. A profiler run is still one process today, but the controller now reads meaning out of what that process prints, and the machinery to run several stages in order is in place and tested behind the existing single-stage API.
Reading the tool's output
ProfilerScrapeEnginematches a controller-owned rule table against the child's console as it arrives. Matching is per complete line: partial output is held until a\ror\narrives, so a rule cannot match across a boundary that the tool has not finished writing, and ANSI escapes are stripped first so a coloured path is still a path. A line over 4 KiB is skipped rather than buffered without limit, and the run reports how many lines it had to skip when a key goes unmatched, so a missing value is never silently attributed to the tool.Rules live in
rocprofvis_controller_profiler_scrape_rules.cpp, keyed by tool, operation and version, with a fallback to the base rules for an unrecognised version. Keeping them in the controller rather than in each backend means the value a run produces does not depend on which caller launched it.A scraped value carries a status rather than just a string:
Pendingwhile the owning stage may still produce it, thenResolved,Unmatched,RuleFailedif the rule itself threw and was disabled, orStageSkippedif the stage never ran. Slots are keyed by stage as well as name, so the same tool in two stages produces two independent values.The rocprof-sys trace rule is split into
Database:, thenFile:, then a bare.dbsweep, so rule order expresses the same preference the View'sParseTraceOutputPathdocuments. Both readers of one log now pick the same file.Running stages in order
A config can hold several stages. Each advances on a zero exit, gets its own executor, and can name the previous stages' scraped values as
{stageN.key}placeholders in its argv, working directory and environment. Tools are resolved once during pre-flight, so a stage is never resolved twice and a missing tool fails the run before anything spawns.A config with no stages runs its flat tool, argv, environment and working directory as a single stage. That is what every caller does today, so the existing path is the one-element case rather than a parallel implementation, and remote launches go through the same bookkeeping so the stage getters behave identically either way.
The controller exposes stage count, per-stage state, the index of the failing stage, and the pipeline's artifact path. The failing index matters for a later UI affordance: when capture succeeds and analyze fails, only the cheap stage needs re-running, and the UI can only offer that if it knows which one broke.
Cancel, the exit code, and the output drain
Cancelpreviously marked slots skipped and returned, leavingUpdateStateto finalise a run it no longer considers running, soEndStagenever ran: a scraped relative path stayed relative and the last unterminated line was never matched. Cancel now drains and ends the stage itself, and the engine ignores anything fed outside aBeginStage/EndStagepair, so the drain that follows the executor cannot resolve a slot just reported as abandoned.This makes cancel semantics explicit, so say so if you disagree: a cancelled stage keeps what it scraped, because a capture killed part way through has still written the file it named. Only stages that never start are marked skipped.
The kill happens with
m_mutexreleased. The executor gives the child a SIGTERM grace period and can then block inwaitpid, which a process parked in a driver call does not leave promptly, andGetOutputtakes the same lock — so holding it would freeze the console the user is watching. A flag set under the lock makes Cancel the sole owner of the ending, so no stage is settled twice and a boundary already in flight cannot start the next one.GetExitCodeis written when a stage ends and read from the UI thread through the C ABI, so it takes the lock now. Cancel sets it too, rather than leaving a cancelled second stage reporting the first one's code.On Windows
ReadOutputdid one 4 KiB read per call, so bytes still in the pipe when the child exited were never read and a chatty tool's console could truncate; it drains until the pipe is empty now. Each executor also kept a second unbounded copy of the whole run's output that nothing ever read.Instrument mode emits a valid command line
rocprof-sys-instrumenttreats-o/--outputas the filename for a rewritten binary and then exits without running the app, and--preset/--traceare registered only onrunandsample, so passing them toinstrumentis a parse error.FlattenToExecutionnow dispatches toFlattenRunOrSampleorFlattenInstrumentRuntime: one-shot runtime instrumentation carries the output folder onROCPROFSYS_OUTPUT_PATHonly and enables Perfetto through the environment instead of the command line.Cancel's result reaches the caller
rocprofvis_profiler_cancelreturned success unconditionally, discarding the controller's answer. It propagates now, andProfilerSessionBase::Cancelreturnsrocprofvis_result_tinstead ofbool, so the orchestrator can tell "stopped it" from "the run finished between the click and the call". The latter previously showed as a spurious "Failed to cancel profiler".Tests
371 assertions in 53 cases on Linux, 223 in 34 on Windows. New coverage for line assembly and the 4 KiB cap, rule versioning and fallback, the shipped rocprof-sys preference order, output arriving after a stage ends, placeholder resolution, stage advancement and skipping, and cancel preserving a stage's scraped values while resolving relative paths against that stage's working directory.
This also fixes a test that has been failing on Linux in main:
A missing working directory fails the run instead of running elsewherestill expected child exit 126 andchdir failed, but the check moved ahead of the spawn, so it is a launch error with the reason on the console. Windows CI never caught it because the test is POSIX-only.rocprofvis_controller_profiler_pipeline_tests.cppis added but deliberately not in the build: it drives the stage C ABI, which does not exist until Phase 2. The loop it would exercise is covered meanwhile byrocprofvis_controller_profiler_pipeline_loop_tests.cpp, which uses the controller's C++ classes directly.Notes for reviewers
Nothing in the View drives a multi-stage run yet. The C ABI gains the stage handle typedef and the operation and scrape-status enums, but not the stage functions, so this cannot change the behaviour of any launch the UI can currently start.
One latent issue is left for Phase 2 rather than fixed here:
UpdateStatestill holdsm_mutexacross artifact relocation at a stage boundary, which could stallGetOutputif a relocation ever moves a large file. It is unreachable today, since relocation only runs for a multi-stage config and nothing can build one.