Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a metric filtering mechanism for GCPerfSim “LowVolatilityRuns” so that only selected metrics are emitted into Results.md / Results.json based on the output.columns list in the YAML configuration.
Changes:
- Updated LowVolatilityRuns YAML configs to use canonical
ResultItemmetric names foroutput.columns/output.all_columns. - Re-enabled metric filtering in GCPerfSim markdown presentation generation.
- Added metric filtering in GCPerfSim JSON presentation generation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/benchmarks/gc/GC.Infrastructure/GC.Infrastructure/Commands/RunCommand/BaseSuite/LowVolatilityRuns.yaml |
Updates the metric lists used for LowVolatilityRuns output selection. |
src/benchmarks/gc/GC.Infrastructure/GC.Infrastructure.Core/Presentation/GCPerfSim/Markdown.cs |
Re-enables filtering so markdown output only includes requested metrics. |
src/benchmarks/gc/GC.Infrastructure/GC.Infrastructure.Core/Presentation/GCPerfSim/Json.cs |
Filters JSON output to only requested metrics. |
src/benchmarks/gc/GC.Infrastructure/Configurations/GCPerfSim/LowVolatilityRuns.yaml |
Mirrors the LowVolatilityRuns metric list updates for the main GCPerfSim configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| string propertyNameToCheck = property.Name.ToLowerInvariant(); | ||
|
|
||
| // TODO: Add the property filter logic back in. | ||
| /* | ||
| if (!requestedPropertyNames.Contains(propertyNameToCheck)) | ||
| { | ||
| //continue; | ||
| continue; |
There was a problem hiding this comment.
Re-enabling this filter makes the output depend on Output.Columns matching ResultItem property names exactly. Many existing GCPerfSim YAML configs still use legacy/friendly column labels like Count, Speed, total pause time (msec), and PauseDurationSeconds_Sum (e.g., Configurations/GCPerfSim/Normal_Server.yaml), which do not correspond to any ResultItem property names, so those metrics will now be silently dropped from Results.md/Results.json for those configs. Consider adding an alias/normalization map (e.g., count→totalnumbergcs, speed→speed_mbpermsec, totalpausetimemsec→pausedurationmsec_sum) and/or emitting a warning when a requested column name doesn’t match any property, or update all affected YAML configs in the same PR to the new canonical metric names.
| if (!requestedPropertyNames.Contains(propertyNameToCheck)) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
With the filter enabled, any Output.Columns entries that don’t match a ResultItem property name (after normalization) will be silently omitted from Results.json. Several existing GCPerfSim YAML configs still use legacy labels like Count, Speed, and total pause time (msec), which don’t map to ResultItem properties, so this change can unexpectedly drop metrics for those scenarios. Consider adding an alias/back-compat mapping (or warning on unmatched column names), or update the other GCPerfSim YAML configs to canonical property names in the same PR.
This pr adds metric filter for LowVolatilityRuns. It decides which metrics will be presented in results.md/results.json.