Improve performance of String.concat for seq of type array or list#9483
Conversation
cartermp
left a comment
cartermp
left a comment
There was a problem hiding this comment.
Some conflicts and stylistic change, but otherwise this looks good!
|
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. |
|
All requested changes have been implemented and build is green. @cartermp / @KevinRansom, is it ok like this? |
|
@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 |
|
Thanks @KevinRansom! |
…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>
3 participants
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.