Skip to content

Conversation

@User0332
Copy link
Contributor

@User0332 User0332 commented Dec 1, 2025

This is related to issue #2838 and PR #2886, and this PR fixes #2838

Previous Behavior

Parameters and arguments passed to benchmarks were grouped into logical groups based on ParameterInstances.ValueInfo (a string representation), which failed for types like arrays (whose string representations just contain length and element type), causing #2838. Any type where objects are not equal but can have the same ToString() was affected by this behavior.

New Behavior

When using DefaultOrderer to group benchmark cases, parameters are compared by their actual value as opposed to their string representations, leading to correct benchmark case grouping. This is done through ParameterComparer, so any parameter or custom wrapping struct for a parameter implementing IComparable will now be properly distinguished regardless of its ToString override. For any IEnumerables (both non-generic and generic), there is now built-in functionality in ParameterComparer which compares enumerables element-by-element for value-based equality.

@User0332
Copy link
Contributor Author

User0332 commented Dec 1, 2025

The failing exporter checks seem to be because I changed the schema of the logical group keys, is there a way I could update what verify is expecting?

E.g. verify expects 'Only 1 job in a group can have "Baseline = true" applied to it, group Invalid_TwoJobBaselines.Foo in class Invalid_TwoJobBaselines has 2' where the logical group key is "Invalid_TwoJobBaselines.Foo" but with my new parameter-grouping keys it outputs 'Only 1 job in a group can have "Baseline = true" applied to it, group Distinct Param Set 0-Invalid_TwoJobBaselines.Foo in class Invalid_TwoJobBaselines has 2' where the logical group key is "Distinct Param Set 0-Invalid_TwoJobBaselines.Foo"

@User0332
Copy link
Contributor Author

User0332 commented Dec 1, 2025

I guess I could have logical groups have a key for grouping and a display name for errors, but I feel this would just add unnecessary nonfunctional code

@timcassell
Copy link
Collaborator

You can just update the verified files.


public bool Equals(ParameterInstances? x, ParameterInstances? y)
{
return ParameterComparer.Instance.Compare(x, y) == 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

2 values who have equivalent comparisons are not necessarily equal. You should rather implement the equality comparer properly.

@User0332
Copy link
Contributor Author

User0332 commented Dec 1, 2025

You can just update the verified files.

Won't this not affect the CI though?
Or should I not worry about that

@timcassell
Copy link
Collaborator

You can just update the verified files.

Won't this not affect the CI though? Or should I not worry about that

I don't understand the question. The test compares the output of the run to the verified file.

@User0332
Copy link
Contributor Author

User0332 commented Dec 1, 2025

You can just update the verified files.

Won't this not affect the CI though? Or should I not worry about that

I don't understand the question. The test compares the output of the run to the verified file.

Sorry, didn't realize that the GitHub actions pulled tests from my PR and not master.

Will implement these changes.

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.

Ratio does not get reset for new input set when using ArgumentsSource

2 participants