Optimize string.join for IList<string> with char separator #56857
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
ghost
added
the
community-contribution
The JIT will optimize the check away for value types. To validate impact here, you'd need to test it against some other reference type. |
|
(For anyone following along, this is based on my comment: #44032 (comment)) |
|
@stephentoub I've replaced benchmark StringJoinIntList with StringJoinClassList and updated the description. |
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Thanks. The small 1-3% regression is unfortunate for the T == non-string reference type cases, but I think it's worthwhile for consistency with the other overloads.
|
Another option would be to add a new overload of Join(char, IEnumerable<String>) to match the Join(String, IEnumerable<String>) method. This would make the type check of T not necessary but would require an an extra JoinCore and the method calls to it to align with the IEnumerable<T> pattern. |
We could consider adding an overload, but it's too late to do that for .NET 6. |
GrabYourPitchforks
left a comment
GrabYourPitchforks
left a comment
There was a problem hiding this comment.
Nit: I'd move the null check to be first and the "is string[]?" check to be before the "is List<string>?" check. This makes the "is string[]?" check blazing fast (it's literally a single dereference and comparison) and moves the relatively more expensive "is List<string>?" to the end. The reason that check is more expensive is that the type is unsealed, which means the runtime needs to emit more complicated logic to prepare for walking a hierarchy. Another thing we could consider is an exact type match (if (foo.GetType() == typeof(List<string>))), which would eliminate the hierarchy checks.
|
Thanks for another perf contribution @johnthcall. We welcome them! |
ghost
locked as resolved and limited conversation to collaborators
4 participants
This is a followup to #44032 because changing from
string,join(",", myList)tostring,join(',', myList)could lead to a performance degradation.The below benchmark includes StringJoinIntList to verify that performance doesn't degrade for joining lists of non-string types.
dotnet/performance@main...johnthcall:StringJoinChar