Skip to content
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

Merged
merged 13 commits into from
Mar 7, 2025

Conversation

msft-paddy14
Copy link
Contributor

@msft-paddy14 msft-paddy14 commented Feb 4, 2025

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.

image

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 |

@msft-paddy14 msft-paddy14 requested a review from vazois February 4, 2025 18:56
@badrishc
Copy link
Collaborator

badrishc commented Feb 4, 2025

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 CLUSTER NODE: https://github.com/microsoft/garnet/compare/badrishc/cluster-node?expand=1

However, the StringBuilder approach is reasonable and simple to begin with, if you are seeing it is sufficient for your use case.

@msft-paddy14
Copy link
Contributor Author

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 CLUSTER NODE: https://github.com/microsoft/garnet/compare/badrishc/cluster-node?expand=1

However, the StringBuilder approach is reasonable and simple to begin with, if you are seeing it is sufficient for your use case.

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?

@badrishc
Copy link
Collaborator

badrishc commented Feb 5, 2025

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 CLUSTER NODE: https://github.com/microsoft/garnet/compare/badrishc/cluster-node?expand=1
However, the StringBuilder approach is reasonable and simple to begin with, if you are seeing it is sufficient for your use case.

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.

@badrishc badrishc self-assigned this Feb 6, 2025
@badrishc
Copy link
Collaborator

@msft-paddy14 - have left a few small comments to address here, thanks.

@vazois vazois merged commit fd22b5e into main Mar 7, 2025
15 checks passed
@vazois vazois deleted the users/padgupta/optimize_stringallocations_clusternodes branch March 7, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants