Skip to content

Conversation

@amokiche-amd
Copy link
Contributor

@amokiche-amd amokiche-amd commented Jan 6, 2026

Motivation

  • Sidebar
    • Improve name labels here as well, for example instead of:

      • Processes(1)
      • [77777]
        - Threads
        • Thread: ./main.py(7777): Thread 7111 (1)

      it should be:

      • Processes(1)
      • ./main.py (77777)
        - Threads
        • Thread 7111 (1)

Technical Details

Change the logic of track name creation
image

Test Plan

Test Result

Submission Checklist

@amokiche-amd amokiche-amd changed the base branch from main to dev January 6, 2026 15:31
@amokiche-amd amokiche-amd changed the title Amokiche/sidebar labels Amokiche/Improve sidebar name labels Jan 9, 2026
@amokiche-amd amokiche-amd marked this pull request as ready for review January 9, 2026 14:09
Copilot AI review requested due to automatic review settings January 9, 2026 14:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request improves the sidebar name labels in the ROCm profiling visualization tool to make process identification clearer. The change updates process display from [77777] to ./main.py (77777), providing both the command name and process ID for better user experience.

Key changes:

  • Refactored track name creation logic into a dedicated GetTrackName() method for better code organization
  • Updated process header labels to include command names alongside process IDs
  • Fixed spelling from "Sample Threads" to "Sampled Threads" for consistency

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/controller/src/rocprofvis_controller_trace.h Adds private method declaration for GetTrackName
src/controller/src/rocprofvis_controller_trace.cpp Implements GetTrackName method to centralize track naming logic and applies it in LoadRocpd
src/view/src/rocprofvis_track_topology.cpp Updates process header to include command name with process ID and corrects "Sample" to "Sampled"
src/view/src/rocprofvis_timeline_view.cpp Renames local variable from m_graphs_reordered to graphs_reordered (non-member variable)
src/model/src/database/rocprofvis_db_sqlite.cpp Removes trailing whitespace
src/model/src/database/rocprofvis_db_rocprof.cpp Removes extraneous blank lines

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@drchen-amd drchen-amd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this matters but since this name is used in the meta area as well, but unlike the side bar, meta area has no nesting to tell what process a track is from.

So maybe if you had multiple processes, meta area becomes harder to tell threads from different processes apart?

Old vs New

Image

@tomk-amd
Copy link
Collaborator

tomk-amd commented Jan 9, 2026

Not sure if this matters but since this name is used in the meta area as well, but unlike the side bar, meta area has no nesting to tell what process a track is from.

So maybe if you had multiple processes, meta area becomes harder to tell threads from different processes apart?

This was my suggestion, to remove the process name from the meta area desc too. One thing that I forgot to mention though was that the process name should remain on the thread that is marked as MAIN.

Perfetto for example does not show the process name for threads:

image

I feel spamming the process name for every thread track is too much? But happy to hear other opinions.

Maybe showing "Thread 154714 : 156619 (2)" would be a compromise Thread proc_id : thread_id so that it still obvious which process the thread belonds to?

@drchen-amd
Copy link
Contributor

I think Perfetto shows short names because their tracks are nested under headers for processes.
image

@drchen-amd
Copy link
Contributor

I think current look for this PR is fine enough. Maybe in the future we could put a colored dot on the sidebar at each process, and put that same colored dot in the meta area for all of a process' tracks.

Copy link
Collaborator

@tomk-amd tomk-amd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the "meta description" area of the tracks in the timeline, let's use the process name instead of the thread name for the "MAIN" thread.

Otherwise looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants