-
Notifications
You must be signed in to change notification settings - Fork 458
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
Refactor: Separate RESP Command Parsing and Execution #164
Conversation
When working on #119 I ran to the problem that the heavy usage of |
Hi @aromaa, we really appreciate your expertise with JIT and inlining to help make this work more optimized, thanks! |
This is great insight! We have not checked yet if the inlining is happening correctly. This should definitely go on our to-do list. I think we'll need a few optimization passes on this - especially on the core loop. So far, we have not tuned the parsing loop much and I am sure there is a lot of room for improvement. We'd definitely appreciate your help with this! |
How do you check the assembly for this, any pointers? |
…rnet into lumaas/parsing-refactor
I recommend using excellent VS extension by member of the .NET JIT team, called Disasmo. warning: it's pretty addicting garnet_disasmo.mp4I believe you need to clone and build the .NET runtime locally in order to use the If VS is not available for you you can follow .NET Runtime guide to try getting the basic JitDisasm manually using As other JIT developers often say, developer should only mark methods with It's very hard to find the right balance between JIT throughput, code size and the resulting inlined assembly which depend very much on callsites and other factors, which is a probably why people experienced in optimizing low-level stuff like this keep telling to measure and not assume something is faster. Here's a random example (there's more) where removing |
…rnet into lumaas/parsing-refactor
This PR is a first step towards a more maintainable RESP parser design.
Overview
This PR tries to separate command parsing and execution by making the following changes:
ParseCommand()
function with clear semantics. If parsing is successful, the function returns theRespCommand
ID, optionally a subcommand ID (if parsed) and the number of remaining elements in the RESP array (i.e., the number of command arguments)RespCommand
enum.Additionally, the parsing code is now enforcing the following semantics on the parsing function and all operators:
Separating parsing and command execution will allow easier integration with components such as ACL and easier replacement of the parsing logic. In addition, it allows us to properly identify any potential bottlenecks in the current parser.
Known Caveats
(RespCommand, byte)
, but most commands still do their own subcommand parsing inside the command executors. This will change in the future with new parser updates.