Improve performance of String.concat for seq of type array or list #9483
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
cartermp
suggested changes
Jun 18, 2020
|
Since we now have three execution paths (for list, seq and array), I changed the tests to execute all three paths for the existing tests 9c6ba3f. |
|
Strange, it keeps saying "Changes requested" even though all requests have been resolved. Maybe I'm doing something wrong... |
|
@abelbraaksma , the requester has to acknowledge that they have been handled I believe. |
KevinRansom
requested changes
Jun 18, 2020
tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs
Outdated
Show resolved
Hide resolved
|
All requested changes have been implemented and build is green. @cartermp / @KevinRansom, is it ok like this? |
cartermp
approved these changes
Jun 25, 2020
|
@cartermp, you merged master into this, I think this is now ready? |
|
@abelbraaksma I think this is ready, but this will require one other review (preferably @KevinRansom since he previously requested changes) |
|
/cc @KevinRansom |
KevinRansom
approved these changes
Jul 25, 2020
|
Thanks @KevinRansom! |
nosami
pushed a commit
to xamarin/visualfsharp
that referenced
this pull request
Feb 23, 2021
…otnet#9483) * Move `String.length` to the top of its module so that the `length` function is in scope * Improve performance of String.concat for arrays and lists * Make concatArray local to String.concat * Testing String.concat, make sure the new three paths are covered * Remove "foo", "bax", "bar" in tests, making them more readable Co-authored-by: Phillip Carter <pcarter@fastmail.com>
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.
This follows the findings and timings shown in this comment: #9390 (comment)
Basically: special-casing the input
seqfor array gives 2x perf gain and less 2-3x less mem use, and special-casing forlistgives 20% gain and 30% less mem and GC pressure.@KevinRansom: this is the one we discussed that shows no improvement by using array-based manipulation if the input is a
seq, and sinceString.Joinuses buffered-SB internally, that may well be the reason it already performed so well.It's also a showcase of the internal
ArrayEnumeratorof arrays-turned-into-sequences, however fast it is, still adding a lot of time for the indirection, hence the success of the special-casing of array. Try this new method withArray.toSeq xand with implicitx :> seq<_>and watch the difference (a factor 2).This PR is dependent on #9481.