Conversation
|
Before we get too far along, I want to flag that this is not compatible with what input and output mean for Tsavorite. All state that we want to pass from application (Garnet session) level to Tsavorite has to be captured in "input". Output corresponds to all state that is returned by the Tsavorite call to the Garnet session (or in future, to a programmatic invocation of the API). The protocol version does not belong to this category, since it is defined at the input level (at the RESP Server Session) and has nothing to do with output state from the Tsavorite call. |
That why I asked.. Ok, what if I use RespInputFlags for this? I could move the SetGet to a lower value, add a Resp3 flag at same bit, and use that for all this. (FlagMask at RespInputHeader will be edited to use new flag, in order to keep same value). The SetGet flag bit is unused for objects, so I can use that while keeping the 'only the last 3 bits are available' thing for objects. |
prvyk
changed the title
|
On commit 2e3e517: Most GarnetApi gets ObjectInput/RawStringInput from caller and we could set the flag there. Some calls do not, and instead set up the input object internally. These did not know the RESP protocol version and so didn't know to put it in the input. Turns out this barely affects a standard case of client connecting via RESP - nearly all functions in use pass input, only test code/custom functions/LCS seem to act this way. Instead of modifying all these APIs, this uses a variable on the storage session. The underlying assumption is that a storage session is never shared between client sessions, so we could just update the storage session each time it changes. The current SWAPDB implementation is limited to a single client, so it's not a problem. Note that the version supplied in the input object (in apis where it's available) always 'wins'. |
prvyk
changed the title
|
Been thinking about this. The changes here have become way too invasive, so we need to think of a simpler approach. The observation is that protocol version is really a slow-changing item (set at the session level) and it is unrelated to individual request calls. So, it does not belong in "input" at all and we are unnecessarily sending this byte back and and forth for every single request call. In fact, for AOF replay at the secondary, when the commands are being processed, we want the backend (we refer to With this in mind, we can follow this approach:
|
Done, also sends the protocol version to Operate(), so we don't need input.arg1 stuff. There are many more places that need adjustments than the one that was using arg1, and having a null protocol would require all of the operations to be aware, so there should be some standard way to pass the protocol version along to the function. |
|
Adding a rough extension idea based on GarnetObjectStoreRespOutput, to refactor the backend state: https://github.com/microsoft/garnet/compare/badrishc/object-store-ideas?expand=1 Whether input should be part of Other thing in this proposal: we keep |
Some form of output consolidation - a wrapper around RespWriteUtils to 'write RESP to memory' - is useful elsewhere too, there's use in LCS here, and in the future IRespSerializable's ToRespFormat() serializations could use a form of this. But that requires making the consolidation independent of the object store. |
Right. the |
|
I think something like this may be desirable (maybe there could be an interface to emit byte[] directly): --- a/libs/server/Resp/RespCommandsInfo.cs
+++ b/libs/server/Resp/RespCommandsInfo.cs
@@ -374,65 +375,73 @@ namespace Garnet.server
/// <returns>Serialized value</returns>
public string ToRespFormat()
{
- if (string.IsNullOrWhiteSpace(this.Name))
- return "$-1\r\n";
+ var output = new SpanByteAndMemory(null); // example
- var sb = new StringBuilder();
- sb.Append("*10\r\n");
+ using (var outputResp = new GarnetObjectStoreRespOutput(2, ref output))
+ {
+ if (string.IsNullOrWhiteSpace(this.Name))
+ {
+ outputResp.WriteNull();
+ }
+ else
+ {
+ outputResp.WriteArrayLength(10);
// 1) Name
- sb.Append($"${this.Name.Length}\r\n{this.Name}\r\n");
+ outputResp.WriteAsciiBulkString(this.Name);
// 2) Arity
- sb.Append($":{this.Arity}\r\n");
+ outputResp.WriteInt32(this.Arity);
// 3) Flags
- sb.Append($"*{this.respFormatFlags?.Length ?? 0}\r\n");
+ outputResp.WriteArrayLength(this.respFormatFlags?.Length ?? 0);
if (this.respFormatFlags != null && this.respFormatFlags.Length > 0)
{
foreach (var flag in this.respFormatFlags)
- sb.Append($"+{flag}\r\n");
+ outputResp.WriteSimpleString(flag);
}
// 4) First key
- sb.Append($":{this.FirstKey}\r\n");
+ outputResp.WriteInt32(this.FirstKey);
// 5) Last key
- sb.Append($":{this.LastKey}\r\n");
+ outputResp.WriteInt32(this.LastKey);
// 6) Step
- sb.Append($":{this.Step}\r\n");
+ outputResp.WriteInt32(this.Step);
// 7) ACL categories
- sb.Append($"*{this.respFormatAclCategories?.Length ?? 0}\r\n");
+ outputResp.WriteArrayLength(this.respFormatAclCategories?.Length ?? 0);
if (this.respFormatAclCategories != null && this.respFormatAclCategories.Length > 0)
{
foreach (var aclCat in this.respFormatAclCategories)
- sb.Append($"+@{aclCat}\r\n");
+ outputResp.WriteSimpleString('@' + aclCat);
}
// 8) Tips
var tipCount = this.Tips?.Length ?? 0;
- sb.Append($"*{tipCount}\r\n");
+ outputResp.WriteArrayLength(tipCount);
if (this.Tips != null && tipCount > 0)
{
foreach (var tip in this.Tips)
- sb.Append($"${tip.Length}\r\n{tip}\r\n");
+ outputResp.WriteAsciiBulkString(tip);
}
// 9) Key specifications
var ksCount = this.KeySpecifications?.Length ?? 0;
- sb.Append($"*{ksCount}\r\n");
+ outputResp.WriteArrayLength(ksCount);
if (this.KeySpecifications != null && ksCount > 0)
{
foreach (var ks in this.KeySpecifications)
- sb.Append(ks.RespFormat);
+ outputResp.WriteAsciiDirect(ks.RespFormat);
}
// 10) SubCommands
var subCommandCount = this.SubCommands?.Length ?? 0;
- sb.Append($"*{subCommandCount}\r\n");
+ outputResp.WriteArrayLength(subCommandCount);
if (this.SubCommands != null && subCommandCount > 0)
{
foreach (var subCommand in SubCommands)
- sb.Append(subCommand.RespFormat);
+ outputResp.WriteAsciiDirect(subCommand.RespFormat);
+ }
}
- return sb.ToString();
+ return output.ToString();
+ }
}
}
}
\ No newline at end of file |
Would this be possible to add to this PR? |
|
There are several ideas here:
Problem is ObjectOutputHeader was used in Disposal, and it seems to need the same curr, end pointers as normal output? Passing these along (in the Dispose) requires a wrapper, and we're back at having some way to have these together (e.g. exposing RespMemoryWriter's pointer to surrounding struct/class). e.g. (Following this, I can take what's left of GarnetObjectStoreRespOutput, turn it into the OperateParams struct and do the rest of it). |
What fraction of the object store calls don't output anything? Wouldn't most calls have to output something? We will need to see if the struct introduces any overhead. |
In List, 50% of operations. In Hash about 40%. Set about a third. In SortedSet/Geo, about a fifth. The ones that don't output almost always set an integer result1 and then output that in the parsing layer. |
prvyk
force-pushed
the
respoutputobject
branch
2 times, most recently
from
d1d12c5 to
89d1fae
Compare
|
I think OperateParams belongs in a different PR, it's a too much churn here. I've been adding RESP3 to COMMAND DOCS/INFO output. It works, the only remaining issue there is that the previous code had a cache of sorts for every command serialization and this one doesn't. IMHO the cache is mostly useless and the caller can do it if they really care. There may be a tiny benefit in caching entire COMMAND DOCS output (about 160K RESP2 uncompressed. redis-cli always fetches it first thing). It's pretty quick as is though - faster than it was, turns out the gain of avoiding strings outweighs the cache, and most first-run time was on constructing the command docs which are later reused - and I'm not sure where to place such a cache (RespServerSession? IGarnetServer?). Otherwise the PR is ready for review. |
|
I agree OperateParams should be separate (if we decide that is worth it). However, a PR with the below change needs to get in before this one:
With this change, the current PR can become much cleaner, e.g., I wanted to do this but have been swamped for the last few days. Will try to get to it asap (around May 1). Feel free to take a stab at it if you wish, but I do understand this is a bit invasive. |
|
Did the ObjectOutputHeader change. A great idea as usual. I didn't understand earlier why the code has to emit an extra header, now I get the adhoc union code. Removing this makes the code far cleaner, and no need for GarnetObjectStoreRespOutput anymore. When something like dotnet/csharplang#8928 goes in, the union could be implemented cleanly if desired. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
I have been using the time to add some more cases to RESP3 update. The PR never claimed to be a complete RESP3 update, and it still isn't. Aside from whatever I've missed:
(After some thought, doing custom functions and modules is better as part of a future PR, since it changes function signatures others may rely on.)
B) Replace just IMemoryOwner (7d12671).
None of these are blockers IMHO. |
|
Great work. Have you checked BDN performance for any regression compared to main? |
|
Running BDN takes a lot of time, and I'm not sure the delta measures more than the temperature of the laptop when it's run... (Also, the HMSet memory result is not replicated in a separate run). It would be nice if someone could set up a more complete and more objective comparison. @darrenge can you please assist here? Before:
After:
|
…Then remove GarnetObjectStoreRespOutput and convert all uses to RespMemoryWriter.
Convert GEOSEARCHSTORE implementation to use RespMemoryWriter. (All sites calling TryReadErrorAsString already checked for '-' character, it's fine to return false there)
…Base. Allow long cursor for object SCAN commands.
…e can have a 64bit argument there.
Improve RespMemoryWriter.
…ObjectStoreOutput).
…zation." This reverts commit f60d08d.
This reverts commit 8139e67.
5 participants
This PR corrects many cases of RESP2 output in RESP3 mode. It does four things:
A) In the parsing layer, many cases were updated to use RESP3 NULLs.
B) Storage session and Tsavorite usually did not know the version so couldn't adjust output.
Set protocol version at the storage session's functionsstate, this is passed along to Operate().
Various cases of NULL/Double/Map/Set output are corrected.
C) Internals often converted to use an output struct to make output code much less noisy. Compared to previous code, I suspect the struct allocates slightly more (padding?) but faster (once).
D) Small miscellaneous RESP2 output fixes.
[Original PR used GarnetObjectStoreOutput, hence badrishc's comment, later we tried RespInputFlag before settling on this approach]