Longer strings for string.Replace(char, char) benchmarks #2329
Conversation
This file contains 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
gfoidl
commented
Mar 23, 2022
| public string Replace_Char(string text, char oldChar, char newChar) | ||
| => text.Replace(oldChar, newChar); | ||
|
|
||
| public static IEnumerable<object[]> ReplaceArguments() |
There was a problem hiding this comment.
Verifyed that the method names are displayed the same way, so that the reporting tool has correlation. Is this enough to be on the safe side here?
| Method | text | oldChar | newChar | Mean | Error | StdDev |
|---------------- |--------------------- |-------- |-------- |-----------:|-----------:|----------:|
| Replace_CharNew | ----(...)---- [100] | - | + | 32.553 ns | 21.554 ns | 1.1815 ns |
| Replace_CharNew | ----(...)---- [1000] | - | + | 202.048 ns | 178.291 ns | 9.7727 ns |
| Replace_Char | Hello | a | b | 5.256 ns | 4.023 ns | 0.2205 ns |
| Replace_CharNew | Hello | a | b | 5.367 ns | 2.158 ns | 0.1183 ns |
| Replace_Char | Hello | l | ! | 17.491 ns | 4.643 ns | 0.2545 ns |
| Replace_CharNew | Hello | l | ! | 18.092 ns | 12.480 ns | 0.6841 ns |
| Replace_Char | This (...)tence [28] | i | I | 36.695 ns | 91.723 ns | 5.0277 ns |
| Replace_CharNew | This (...)tence [28] | i | I | 30.385 ns | 25.207 ns | 1.3817 ns |
| Replace_Char | This (...)tence [28] | z | y | 11.598 ns | 6.366 ns | 0.3489 ns |
| Replace_CharNew | This (...)tence [28] | z | y | 12.132 ns | 10.304 ns | 0.5648 ns |
The ones with suffix New are the methods with the ArgumentsSource, the other ones with Arguments only (as in main-branch).
There was a problem hiding this comment.
@adamsitnik @DrewScoggins can you confirm this won't reset the results?
There was a problem hiding this comment.
Sorry, I thought I had already responded here. As long as the test name or the arguments have not changed values we should have no issues here. And from what I can tell nothing has changed for the existing test cases. So this LGTM.
danmoseley
reviewed
Mar 23, 2022
Co-authored-by: Dan Moseley <[email protected]>
danmoseley
approved these changes
Mar 23, 2022
DrewScoggins
approved these changes
Mar 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Cf. dotnet/runtime#67049 (comment)
/cc: @danmoseley