Adding wrongType bool to GarnetObjectStoreOutput #923
Conversation
|
Why isn't allocated at zero after this change? Need to check what else gets allocated on critical path. There doesn't logically seem to be anything else needed. |
| @@ -47,7 +47,11 @@ public bool InitialUpdater(ref byte[] key, ref ObjectInput input, ref IGarnetObj | |||
| if ((byte)type < CustomCommandManager.CustomTypeIdStartOffset) | |||
| { | |||
| value = GarnetObject.Create(type); | |||
| value.Operate(ref input, ref output.spanByteAndMemory, out _, out _); | |||
| value.Operate(ref input, ref output.spanByteAndMemory, out _, out _, out var wrongType); | |||
There was a problem hiding this comment.
Operate should take in ref GOSO instead of adding another out param. cc @TedHartMS
| /// <returns></returns> | ||
| bool Operate(ref ObjectInput input, ref SpanByteAndMemory output, out long sizeChange, out bool removeKey); | ||
| bool Operate(ref ObjectInput input, ref SpanByteAndMemory output, out long sizeChange, out bool removeKey, out bool wrongType); |
There was a problem hiding this comment.
Please clean this up if possible, remove size change and remove key. Put into enum perhaps?
| /// <summary> | ||
| /// True if an operation was attempted on the wrong type of object | ||
| /// </summary> | ||
| public bool wrongType; |
There was a problem hiding this comment.
We discussed making this an enum, please check that...
There was a problem hiding this comment.
Oh I missed that comment :) Can do!
| "expected_ModuleNoOpRawStringReadCommand_AOF": 0, | ||
| "expected_ModuleNoOpRawStringRmwCommand_AOF": 0, | ||
| "expected_ModuleNoOpObjRmwCommand_AOF": 8800, | ||
| "expected_ModuleNoOpObjReadCommand_AOF": 5600, | ||
| "expected_ModuleNoOpObjRmwCommand_AOF": 3200, |
There was a problem hiding this comment.
Scope of this PR may be a bit narrow, IMHO. Better if one PR handles all the technical debt related to custom ops perf, since each individual item is rather straightforward, technically.
There was a problem hiding this comment.
I think this one is self-contained and unrelated to the changes which have to do with the CustomCommandManager, makes it easier to review and pinpoint to a certain commit if something needs to be debugged / reverted.
This change is to avoid checking for
GarnetObjectStoreOutput.spanByteAndMemory.Length == 0in order to determine if object operation was attempted on a mismatched type (and by that to allow object operations to return an empty output and avoid unnecessary allocations)Benchmarks for module operations after the change:
Note: # of allocated bytes decreased for ObjRmw, ObjRead & JsonSet.