-
Notifications
You must be signed in to change notification settings - Fork 53
refactor uncore event collection and handling #462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
There was a problem hiding this 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 PR refactors the uncore event collection and handling pipeline to eliminate per-device event expansion, which was causing "Argument list too long" errors with perf. The refactoring simplifies event processing by aggregating uncore events at collection time rather than expanding and later collapsing them.
- Removes uncore event expansion and abbreviation logic that was previously used to manage long argument lists
- Introduces event bucketing and aggregation functions to handle uncore events more efficiently
- Separates event parsing from group assignment for clearer code organization
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/metrics/event_defs.go | Removes event name abbreviation and uncore group expansion functions, along with unused imports |
| cmd/metrics/event_frame.go | Replaces coalesceEvents with bucketEvents and adds aggregateUncoreEvents function; removes legacy uncore group collapsing logic |
| cmd/metrics/metric.go | Improves error handling for metric variable assignment and clarifies C-state residency adjustment logic |
| cmd/metrics/metric_defs.go | Removes abbreviation of event names in metric expressions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
Expanding uncore events to per-device events creates a very long list of event groups. In some cases the list is too large for perf to handle resulting in an "Argument list too long" error from perf. We have worked around this in the past by abbreviating event names to shorten the argument list.
Through investigating the problem above, we found that the individual uncore device event counters are not used independently. They are added together, and the sum is used. The original intent of uncore event expansion is unknown.
In this PR, we eliminate the expansion of uncore events.
Copilot Description Follows
This pull request refactors the metrics event processing pipeline to simplify event handling and improve aggregation logic, especially for uncore events. It removes legacy abbreviation and group expansion logic, consolidates event bucketing and aggregation steps, and clarifies group assignment for metric variables. These changes streamline the codebase and make the event handling more explicit and maintainable.
Event bucketing and aggregation improvements:
coalesceEventsfunction withbucketEventsand addedaggregateUncoreEventsto sum and deduplicate uncore events at socket granularity, removing the need for post-processing uncore group collapsing. (cmd/metrics/event_frame.go) [1] [2]collapseUncoreGroupsInFrameand related helper functions, as uncore event aggregation is now handled earlier in the pipeline. (cmd/metrics/event_frame.go)Group assignment and parsing changes:
assignEventsToGroupsto explicitly assign events to groups based on event group definitions after aggregation, separating parsing and grouping logic. (cmd/metrics/event_frame.go) [1] [2]parseEventsto only unmarshal JSON and parse values, removing abbreviation and group assignment logic from this step. (cmd/metrics/event_frame.go) [1] [2]Removal of legacy abbreviation and expansion logic:
cmd/metrics/event_defs.go,cmd/metrics/metric_defs.go) [1] [2] [3] [4]Metric variable assignment clarification:
cmd/metrics/metric.go)Dependency cleanup:
regexpandslicesfromcmd/metrics/event_defs.go.