-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace String concat with StringBuilder in Cluster Nodes #994
Replace String concat with StringBuilder in Cluster Nodes #994
Conversation
Ideally, there would be no need to create strings at all, or use stringbuilder, if this is a very common operation. We could directly write to a session-local scratch buffer that is reusable for that session, and write to it a sequence of the usual style of writing: while (!RespWriteUtils.TryWriteXxx(...))
... Here is a rough (non-working) prototype of this for However, the |
Your approach seems more performant. However, since it's relatively more code and needs few interface changes, maybe string builder is okay to start with for simplicity and more gains compared to main code. We can optimize further with scratchbuffer if needed? |
Yeah this is at least better than main. |
@msft-paddy14 - have left a few small comments to address here, thanks. |
… of https://github.com/microsoft/garnet into users/padgupta/optimize_stringallocations_clusternodes
StackExchange.Redis does periodic CLUSTER NODES and for large clusters a lot of allocations can happen with such calls.
We recently saw an issue where lots of allocation increased our .NET committed memory and also GC activity when new clients would be deployed/control plane would probe nodes where cluster size was ~100 nodes.
Using StringBuilder in an adhoc BDN test shows we can get significant improvements using this
| TestMultiStringConcat | None | 125.13 us | 1.894 us | 2.395 us | 93.5059 | 4.3945 | 1567640 B |
| TestMultiStringConcatBuilder | None | 41.59 us | 0.627 us | 0.586 us | 9.5215 | 0.9155 | 160152 B |