Skip to content

Instrumentation refactoring cleanups & UX improvements#193

Merged
AlexJones0 merged 4 commits into
lowRISC:masterfrom
AlexJones0:instrumentation_refactor_3
May 14, 2026
Merged

Instrumentation refactoring cleanups & UX improvements#193
AlexJones0 merged 4 commits into
lowRISC:masterfrom
AlexJones0:instrumentation_refactor_3

Conversation

@AlexJones0
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 commented May 13, 2026

This PR is the fourth of a series of PRs to introduce instrumentation reporting to DVSim, which in this case also involves refactoring the existing instrumentation implementation to allow better modeling & static typing around this use case.

This PR performs some final small cleanups after the instrumentation refactoring work in the preceding PRs. Alongside removing some duplication of the instrumentation names, it primarily focuses on improving the user experience:

  1. Make metadata an explicit instrumentation type, to replace the unintuitive behaviour where it is automatically added alongside other instrumentation types.
  2. Rename the resources instrumentation to compute, to make it clear that we are measuring "compute resources" and not the new "resources" concept that was introduced to the scheduler in Augment the scheduler with resources to allow more fine-grained parallelism limitation #184 for more granular parallelism limits.
  3. Add a special all instrumentation type which automatically includes all the available instrumentations. This lets you type --instrument all instead of --instrument meta timing compute, which can get overly verbose.

See the commit messages for more info.

@AlexJones0 AlexJones0 changed the title Instrumentation refactoring cleanups + UX improvements Instrumentation refactoring cleanups & UX improvements May 13, 2026
Copy link
Copy Markdown
Collaborator

@machshev machshev left a comment

Choose a reason for hiding this comment

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

Nice, thanks @AlexJones0!

Reduce duplication of naming across the instrumentation logic to try and
keep a single source of truth, which will be useful when trying to load
back JSON instrumentation reports.

Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Users may want the ability to choose whether metadata should be included
in instrumentation reports or not - it may not be relevant to their use
case, and bloat the size of the generated reports for large runs.

Also, the previous implicit behaviour where we automatically include it
if you use any other type of instrumentation is not particularly nice or
intuitive. Now, users must implicitly include metadata instrumentation
with the "meta" type. As the unique `job_id`, the full name will
necessarily always be included in the output instrumentation report if
any type of instrumentation is used. But all other metadata must be
explicitly included.

Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
It can be arduous to define all the instrumentation types when a
relatively common pattern will be to just collect all the data.
Introduce a special 'all' keyword that internally maps to just using all
instrumentation types that are available.

Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Since this "resources" instrumentation mechanism was added, resources
have been separately introduced to DVSim as the concept of resource
proxies used by jobs to define more granular runtime parallelism limits.
Now the naming and command-line options are overlapping somewhat.
"Resources" was also a bit of a generic name anyway, so lets rename this
to "compute" - short for "compute resources" - to more accurately
reflect what we're capturing (and to make it more distinct).

Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
@AlexJones0 AlexJones0 force-pushed the instrumentation_refactor_3 branch from b6374d6 to 288138c Compare May 14, 2026 11:00
@AlexJones0 AlexJones0 marked this pull request as ready for review May 14, 2026 11:00
@AlexJones0
Copy link
Copy Markdown
Contributor Author

The latest force push is a rebase on master now that #189 and #192 have been merged, with no other changes.

@AlexJones0 AlexJones0 added this pull request to the merge queue May 14, 2026
Merged via the queue into lowRISC:master with commit e202d9a May 14, 2026
6 checks passed
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.

2 participants