runtime
runtime copied to clipboard
dotnet
shortcut string replace if oldvalue is longer than count
thoughts on this small optimization? or do i have my understanding incorrect?
given StringBuilder.Replace knows the max characters range to replace, is it safe to early exit if the oldValue.Length is greater than that range count?
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.
![dotnet-issue-labeler[bot] avatar](https://avatars.githubusercontent.com/in/90031?v=4 "Published by a pub.dev verified publisher"){kind=small}
Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.
Issue Details
thoughts on this small optimization? or do i have my understanding incorrect?
given StringBuilder.Replace knows the max characters range to replace, is it safe to early exit if the oldValue.Length is greater than that range count?
| Author: | SimonCropp |
|---|---|
| Assignees: | - |
| Labels: |
|
| Milestone: | - |
![msftbot[bot] avatar](https://avatars.githubusercontent.com/in/26612?v=4 "Published by a pub.dev verified publisher"){kind=small}
This seems like a safe early exit to me, but I'm curious how common this case is to warrant the added complexity of the early exit code. Are there real-world applications to passing a count smaller than the length of the word we want to replace?
@dakersnar in my case i have some global cleanups that run as part of snapshot testing. these replacements run on all string properties as part of taking a json snapshot of a model. so for example replacing the full current directory path with a token. in the majority of cases (ie any string short than that paths length) it should be a no-op.
as for how common in other real-world applications... no idea. having said that it is a very small amount of code and a single number check will not be measurable (perf wise) in all other cases
I'm not so sure it won't be measurable- we have seen such things regress hot paths (which this is) eg by making code longer so it doesn't inline any more. So if it's a small win in a small proportion of cases it's not clear to me it's worth it. I'd be inclined to agree that it would be reasonable to add this check to calling code if it's doing this regularly.
There are various rules and heuristics, and it may depend on the caller. Cc @AndyAyersMS for thoughts
@danmoseley is a 63line method a candidate for inlining?
There are various rules and heuristics, and it may depend on the caller. Cc @AndyAyersMS for thoughts
Probably not, though it may be contextual (eg oldValue or newValue are literal strings at some call sites).
How much IL is this (before/after)?
cc @EgorBo
While I appreciate the contribution, this doesn't seem generally valuable enough to take. I can't find meaningful examples where this would kick in (can you point to some call sites on GitHub this would help?), the caller itself can do the exact same thing (and save even more overhead), and every caller of this method is forced to incur the additional check and branch.
