string.Join small optimization #8346
Conversation
There was a problem hiding this comment.
Nice changes! However, you should post numbers when you're making perf optimizations, to make sure the functions you're changing are actually speeding up. I have posted instructions for doing this previously at #3163 (comment).
| { | ||
| if (value == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(value)); | ||
| } | ||
|
|
||
| return Join(separator, value, 0, value.Length); | ||
| return JoinCore(&separator, 1, value, 0, value.Length); |
There was a problem hiding this comment.
@AlexRadch I am unsure if changing the code like this will make things faster. When the code is run, the JIT compiler should inline this Join call, so that it would be effectively the same as writing JoinCore(&separator, 1, value, 0, value.Length).
There was a problem hiding this comment.
If we add some checks or something else inside Join(separator, value, 0, value.Length) later, then JIT compiler should make call not inline. So I suggests use JoinCore(&separator, 1, value, 0, value.Length) here. Also code looks more monotonous in different Join methods.
There was a problem hiding this comment.
What are the actual performance gains from skipping this method?
| { | ||
| if (value == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(value)); | ||
| } | ||
| return Join(separator, value, 0, value.Length); | ||
| if (string.IsNullOrEmpty(separator)) | ||
| return string.Concat(value); |
There was a problem hiding this comment.
👍 This should help perf, since Concat has to do less bookkeeping than Join. Please use braces here, though.
if (string.IsNullOrEmpty(separator))
{
return string.Concat(value);
}| } | ||
|
|
||
| [ComVisible(false)] | ||
| public unsafe static string Join(string separator, params object[] values) | ||
| { | ||
| separator = separator ?? string.Empty; | ||
| if (string.IsNullOrEmpty(separator)) | ||
| return string.Concat(values); |
|
|
||
| return StringBuilderCache.GetStringAndRelease(result); | ||
| // Defer argument validation to the internal function | ||
| return JoinCore(pSeparator, separator.Length, values); |
There was a problem hiding this comment.
This is now calling a generic function to do the work, which can be slower in some circumstances. Have you measured if the new implementation was faster?
There was a problem hiding this comment.
It can be a little slower on calling ToString() method that does nothing for strings. So I think it does not downgrade performance but remove copy paste code from source.
There was a problem hiding this comment.
@AlexRadch could you provide some numbers before and after this change?
| // If the separator is null, it is converted to an empty string before entering this function. | ||
| // Even for empty strings, fixed should never return null (it should return a pointer to a null char). | ||
| Contract.Assert(separator != null); | ||
| Contract.Assert(separatorLength >= 0); |
There was a problem hiding this comment.
Should this be separatorLength > 0 instead? It looks like after your changes, Concat is called if the separator is null or empty.
There was a problem hiding this comment.
Now it true but method works correct if separatorLength equal 0, so why it should assert if separatorLength equal 0. Also third JoinCore method is called with separatorLength equal 0. So I think better to have the same asserts in all JoinCore methods.
There was a problem hiding this comment.
@AlexRadch If someone makes changes to this method in the future, they won't have to do any special handling of empty strings because they would know that separatorLength > 0.
| // If the separator is null, it is converted to an empty string before entering this function. | ||
| // Even for empty strings, fixed should never return null (it should return a pointer to a null char). | ||
| Contract.Assert(separator != null); | ||
| Contract.Assert(separatorLength >= 0); |
There was a problem hiding this comment.
Same comment here. edit: I meant in the other JoinCore method.
|
@AlexRadch thanks for working on this! Could you please provide some performance measurements with and without your changes? Without that it is difficult to see the impact your change has on the performance of Join. |
|
@AlexGhiondea How to create performance test for |
I have posted instructions previously at #3163 (comment). |
|
Where are there string join performance tests that I can execute this old and with changed code? |
|
@AlexRadch I do not believe there are any, the approach is to make a throwaway test using the approach like @jamesqo. Please try that out and post before/after numbers for various scenarios. |
|
@danmosemsft Write correct performance tests is hard work and it takes long time. I do not know when I can do such tests. Sorry. |
|
I do not believe it needs to be complex, eg., this one is simple: #3163 (comment) Your call though! |
|
@AlexRadch I agree with @danmosemsft; it is not that hard once you get the hang of it. All you have to do is write something like this using System.Diagnostics;
class Program
{
// Change this depending on how long the method you are testing takes.
const int Iterations = 10000000;
static void Main()
{
for (int i = 0; i < 10; i++) // Run the benchmark 10 times
{
var sw = Stopwatch.StartNew(); // Start a new stopwatch while we're benchmarking
for (int j = 0; j < Iterations; j++)
{
TheMethodYouAreTestingThePerformanceOf();
}
sw.Stop(); // Stop the stopwatch, now that we've finished benchmarking
Console.WriteLine(sw.Elapsed); // Write how long it took to do the benchmark
}
}
}And follow the instructions here for running the performance test. |
|
@AlexRadch do you expect to give this a shot? It would be interesting to verify what improvements this gives. |
|
This PR has not been updated in a month, closing. Would like to see any perf improvement for sure, feel free to reopen/create new PR if someone wants to continue here. |
string.Join small optimization by calling string.JoinCore