Skip to content

Conversation

caaavik-msft
Copy link
Contributor

@caaavik-msft caaavik-msft commented Aug 5, 2025

Due to how the FullNameProvider works, the Dictionary<string, bool>[] and Dictionary<List<int[]>[,], List<int?[][][,]>>[] arguments were both being converted to the string System.Collections.Generic.Dictionary`2[]. Patching this in BenchmarkDotNet would cause too many test names to change, and this is the only case in this repo that there is a duplicate full name, so we can address it by wrapping the type and overriding the ToString.

The display names that I have put here ensure that the test names remain the same as they were before, with only the Dictionary<List<int[]>[,], List<int?[][][,]>>[] case now being converted to typeof(System.Collections.Generic.Dictionary`2[]) (COMPLEX) to differentiate it from the simpler case.

I have also updated the validators to ensure this doesn't happen again.

@caaavik-msft
Copy link
Contributor Author

@adamsitnik As you had originally added this benchmark, are you able to confirm that the changes to the benchmark by using a wrapper type are acceptable and that adding the property lookup should have a negligible impact on the performance measurement?

Copy link
Member

@LoopedBard3 LoopedBard3 left a comment

Choose a reason for hiding this comment

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

Looks good.

@DrewScoggins
Copy link
Member

@caaavik-msft We should probably go ahead and merge this, yeah? Also did we do work on the reporting side to coalesce the results into one trend?

@caaavik-msft
Copy link
Contributor Author

@caaavik-msft We should probably go ahead and merge this, yeah? Also did we do work on the reporting side to coalesce the results into one trend?

Ah yes, we can merge it, although I don't think there will be a way to fix this on the reporting side, we have no way of knowing which test result came from which, but it will be fixed from now on. We may get a perf regression from this now that the test will stabilise on one case rather than being random

@caaavik-msft caaavik-msft merged commit 81ac4c9 into dotnet:main Aug 26, 2025
73 of 80 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.

3 participants