Refactor instrumentation interfaces to match the new scheduler#189
Open
AlexJones0 wants to merge 3 commits into
Open
Refactor instrumentation interfaces to match the new scheduler#189AlexJones0 wants to merge 3 commits into
AlexJones0 wants to merge 3 commits into
Conversation
This abstraction made sense when the instrumentation was integrated within the scheduler - now that it hooks in through callbacks, this is just extra complexity and needlessly hurting performance. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
This commit mostly refactors and moves around existing logic. The CompositeInstrumentation interface was not ideal: - In reality, we have no need for arbitrarily nesting a hierarchy of instrumentation. Just the ability to compose multiple together is sufficient. - Arbitrary composition meant that we had to generate reports in a structure such that it is not immediately clear which instrumentation data came from where - this makes it harder to introduced Pydantic models with appropriate static typing, which we want to do. - Additional overheads for callbacks - instead of an extra function call for every callback, the new aggregator just sets up all the callbacks to the instrumentation implementations directly instead. - Better encapsulation: the aggregator/manager can now own the setup/stop lifecycle and the report collection and creation/merging logic. Essentially, this should allow further refactoring with better modelling and static typing, which will be useful for doing more with this feature. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Now that there has been adequate refactoring to introduce a stable `id` property to the `JobSpec` model for the scheduler rewrite, we can make use of it here instead of the combination of the full name and target. This helps move towards a single clear source of truth for job identity. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
This was referenced May 13, 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
1 participant
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.
This PR is the first 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 initial refactoring to the instrumentation logic to make improvements that are now possible due to the move to the new async scheduler interface. Notably:
NoOpInstrumentation- we can just not register callbacks instead.CompositeInstrumentationwith aninstrumentationAggregatorinterface - there is no need for arbitrary levels of nesting in instrumentation, and it will make introducing pydantic models later more difficult.JobSpec.idproperty introduced alongside the new scheduler rather than re-defining an unstable tuple on the job fields everywhere.This should facilitate the movement to static/frozen Pydantic models for instrumentation data, which will mean we can define well-typed instrument report generation logic in later PRs. See the commit messages for more details.