Skip to content

[DO NOT MERGE] Explore consistency and performance frameworks for writing responses#1781

Draft
kevin-montrose wants to merge 16 commits intomainfrom
users/kmontrose/responseExploration
Draft

[DO NOT MERGE] Explore consistency and performance frameworks for writing responses#1781
kevin-montrose wants to merge 16 commits intomainfrom
users/kmontrose/responseExploration

Conversation

@kevin-montrose
Copy link
Copy Markdown
Contributor

@kevin-montrose kevin-montrose commented May 8, 2026

For discussion and PoC purposes, explore adding some analyzers to make use consistent in how we deal with RESP output.

This PR is not intended to be merged, though the analyzers might be salvageable.

The primary motivation is correctness, but there might be some performance improvements to be had as well.


Analyzers

Motivating incident is #1776 where we were incorrectly handling large outputs.

Introduces 7 diagnostics over 3 analyzers in Garnet.server:

  1. Avoid direct use of RespWriteUtils
  • Instead use the helpers in libs\server\Resp\RespServerSessionOutput.cs
  • Motivation here is to be consistent with how we write output
  1. Use Large overrides with variable size responses
    • If a response cannot provably fit with the minimum send buffer, we have to handle variable size output
    • Several, but not all, of the 100-ish places this is violated are real bugs like Verbatim String Fixes #1776
  2. Add Large overrides for variable size responses
    • Helper for 2, where an override does not yet exist
  3. Move output constants to CmdStrings
    • Motivation is to be consistent, and enable other analyzers like 3, 6, & 7
  4. Use existing constants in CmdStrings
    • Helper for 4, so we don't duplicate a ton of constants
  5. CmdStrings constant too large
    • Limit is pulled off of CmdStrings.MaximumConstantSize
    • Motivation is to make sure we don't build up constants so big that we need to use the Large overrides for them
  6. Collapse repeated constants Write calls
    • "Constant" calls are those with no arguments, actual constant arguments from C#'s perspective, or only CmdStrings arguments
    • There are about a dozen of these cases, though more will presumably arise over time
    • _Motivation is to minimize inlining pressure (from multiple copies of the WriteXXX methods) and runtime work of things like NumDigits(...) and SendAndReset()

These analyzers are quick and dirty so they fit our current coding practices, but could certainly be made more robust to future changes.


Other Work Not Explored

To really adopt/benefit from all this we'd also want to:

  • Remove the AbortXXX helpers, directly using the WriteXXX methods everywhere
  • Audit all the WriteLargeXXX calls and move those that are probably bounded to normal Write calls (presumably with a Debug.Assert about the expected maximum length)
  • Expand to include Garnet.cluster & Garnet.common, probably after refactoring some things to move CmdStrings and the WriteXXX helpers a to Garnet.common
  • Actually forcing the send buffer to be at least CmdStrings.MaximumConstantSize

Potential Optimizations

These are not implemented, but are possible if we pursue this work:

  • [MethodImpl(MethodImplOptions.NoInlining)] on uncommon helpers (presumably all error paths)
    • Our inlining budget is real tight at the moment, this may releave some pressure
  • A branch-less SendAndReset() for non-WriteLargeXXX methods
    • The current implementation has to consider if there are bytes pending before called, and raises an error if not (as we're in the error case tripped by Verbatim String Fixes #1776) - but with known sizes we know that case is unreachable statically
    • This implementation would also be slightly smaller, which may improve code gen
  • Similarly known maximums would allow smaller implementations of utilities like NumUtils.CountDigits when using non-WriteLargeXXX methods
  • We could remove a back branch as well, though whether that's a win is hard to know in advance
    • The pattern would be to do if(TryWriteXXX(...)) { SendAndReset(); WriteXXXUnlikely(...); } where TryWriteXXX and SendAndReset() are aggressively inlined and WriteXXXUnlikely is no inlined

There's also potential waaaay down the line to look at Interceptors to clean all this up - in particular we could remove CmdStrings in its entirety, and automate the collapsing of repeated constant calls.

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.

1 participant