Micro-optimisations: Improve memory usage for ReplaceMany string extension method - #23483
Conversation
|
Hi there @patrickdemooij9, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
ReplaceMany string extension method
AndyButland
left a comment
There was a problem hiding this comment.
Thanks for this @patrickdemooij9 - looks good and worth having. I've just pushed an update with an increase in test coverage, and verified them on the before and after code.
…tension method (#23483) * Improve memory usage for ReplaceMany * Small improvement * Use actual implementation in benchmark. * Further test cases to increase coverage and verify no risk of regression. --------- Co-authored-by: Andy Butland <abutland73@gmail.com>
|
Cherry-picked to |
Prerequisites
Description
The old ReplaceMany was using multiple .Replace() functions, each of them creating a new string in memory. As it's just a char for char replacement, we can change this by constructing a single string and replacing them as we are constructing the string resulting in only one string being allocated.
I also reran all the benchmarks for the ReplaceMany and updated the timings (which also shows DotNet is just a lot faster in general :P). Main ones regarding this code are:
short text, short replacement:
long text, short replacement: