Improvements in command handling #5 #631

TalZaccai · 2024-09-03T19:59:58Z

Benchmarking results:
main:

Method	Mean	Error	StdDev	Allocated
InlinePing	1.493 us	0.0225 us	0.0188 us	-
Set	8.963 us	0.1783 us	0.1751 us	-
SetEx	13.153 us	0.2306 us	0.4388 us	-
Get	5.804 us	0.0819 us	0.0726 us	-
ZAddRem	73.947 us	1.1658 us	1.0334 us	23552 B
LPushPop	80.001 us	1.5987 us	3.0801 us	30721 B
SAddRem	64.207 us	1.2544 us	2.3560 us	16384 B
HSetDel	79.286 us	1.0700 us	1.0009 us	55297 B
MyDictSetGet	116.929 us	1.1018 us	0.9200 us	30720 B

current branch:

Method	Mean	Error	StdDev	Allocated
InlinePing	1.492 us	0.0185 us	0.0173 us	-
Set	8.841 us	0.1280 us	0.1198 us	-
SetEx	13.146 us	0.1434 us	0.1341 us	-
Get	6.049 us	0.0763 us	0.0714 us	-
ZAddRem	71.771 us	1.2070 us	1.1291 us	23552 B
LPushPop	71.520 us	1.2398 us	1.1597 us	30721 B
SAddRem	59.022 us	0.7709 us	0.7211 us	16384 B
HSetDel	76.690 us	1.2125 us	1.0748 us	55297 B
MyDictSetGet	114.768 us	1.0504 us	0.9825 us	30720 B

does not work

…rnet into talzacc/header_impr

…oft/garnet into talzacc/cmd_handling_impr

libs/server/Resp/HyperLogLog/HyperLogLogCommands.cs

libs/server/Storage/Session/MainStore/HyperLogLogOps.cs

libs/server/InputHeader.cs

vazois · 2024-09-20T21:24:54Z

libs/server/Resp/Bitmap/BitmapCommands.cs

-            *(long*)(pcurr) = bOffset; pcurr += sizeof(long);
-            *pcurr = bSetVal;
-            #endregion
+            var input = new RawStringInput


It seems this pattern is repeated across commands. Does it make sense to do it right after parseState instead of at the time of command execution?

a. When you're setting parseState you don't know the command yet
b. parseStateStartIdx is 1 only when it's a single-key command, so it really depends on the command syntax.
c. What we can do is have a cleaner constructor for RawStringInput (as well as ObjectInput).

vazois · 2024-09-20T21:35:05Z

libs/server/Resp/Bitmap/BitmapCommands.cs

            if (parseState.Count > 4)
            {
                var sbOffsetType = parseState.GetArgSliceByRef(4).ReadOnlySpan;
-                bitOffsetType = sbOffsetType.EqualsUpperCaseSpanIgnoringCase("BIT"u8) ? (byte)0x1 : (byte)0x0;
+                if (!sbOffsetType.EqualsUpperCaseSpanIgnoringCase("BIT"u8) &&


How difficult is to propagate the result of this parsing to RMWMethods? It seems we perform this comparison twice which is not efficient.
Maybe we can have an array int args for those commands that require translation of string to categorical arguments in order to pass them in backend functions and avoid another translation/comparison.

True, we are parsing twice, once for validation and once for execution. I think that's part of the price for simplifying the code and making it more readable... @badrishc any ideas here?

Why can this not be passed as a parameter in input, similar to arg1 etc. and if the backend does not find it (not sure why, maybe because of GarnetAPI code path?), it can reparse it from the parse state.

badrishc

See comments. In general, pls check for all cases where we create an array of structs, such as ArgSlice[] or SpanByte[] -- these are allocations that need to be avoided if possible, by moving them to session-level fields that are reused across calls into Garnet.

libs/server/Resp/Parser/ParseUtils.cs

libs/server/Resp/HyperLogLog/HyperLogLogCommands.cs

libs/server/AOF/AofProcessor.cs

libs/server/Resp/ArrayCommands.cs

libs/server/Storage/Session/StorageSession.cs

libs/server/Storage/Functions/MainStore/PrivateMethods.cs

libs/server/AOF/AofProcessor.cs

libs/server/InputHeader.cs

libs/server/Resp/BasicCommands.cs

TedHartMS

Reviewed to BitmapCommands.cs

TedHartMS · 2024-10-04T01:55:51Z

libs/server/InputHeader.cs

+        /// Get header as Span
+        /// </summary>
+        /// <returns>Span</returns>
+        public unsafe Span<byte> AsSpan() => new(ToPointer(), Size);


Why would we need this as a Span? The only place I see this used is in Length below, which I think could just return Size--and is only used by SpanByte method, so maybe can just be consolidated into that

TedHartMS · 2024-10-04T02:22:03Z

libs/server/InputHeader.cs

+                var serializedLength = header.SpanByte.TotalSize
+                                       + (3 * sizeof(int)) // Length + arg1 + arg2
+                                       + parseState.GetSerializedLength(parseStateStartIdx);
+


why the local var? Can just be an expression =>

TedHartMS · 2024-10-04T02:41:46Z

libs/server/InputHeader.cs

+
+        /// <inheritdoc />
+        public unsafe void CopyTo(byte* dest)
+        {


We should pass in the dest len and verify. I know you've verified the length at the callsite, but even so, we are essentially doing C/C++ buffer-copy and should follow the security guidelines for those by doing bounds-checking, just as earlier secure-code initiatives replaced memcpy with memcpy_s

TedHartMS · 2024-10-04T02:46:54Z

libs/cluster/Server/Migration/MigrateSessionKeys.cs

-                // 1. Header
-                ((RespInputHeader*)pcurr)->SetHeader(RespCommandAccessor.MIGRATE, 0);
+                var input = new RawStringInput();
+                input.header.SetHeader(RespCommandAccessor.MIGRATE, 0);


Can this pass the RespCommand as a ctor arg? The more we guarantee correct initialization, the better

TedHartMS · 2024-10-04T02:59:44Z

libs/common/RespReadUtils.cs

+            var parseSuccessful = TryReadLongSafe(ref ptr, end, out value, out bytesRead, out var signRead,
+                out var overflow, allowLeadingZeros);
+
+            if (parseSuccessful) return true;


better readability to have return on its own line. In fact parseSuccessful is not needed; just return true if TryReadLongSafe. and only the second of the "return false" lines below is needed

TedHartMS · 2024-10-04T16:56:35Z

libs/server/AOF/AofProcessor.cs

+            if (parseStateCount > 0)
+            {
+                parseState.Initialize(parseStateCount);
+


I had to go look to figure out what "count" was here. Better to name it (and the ctor arg) "argCount"

TedHartMS · 2024-10-04T17:00:09Z

libs/server/AOF/AofProcessor.cs

+            var curr = ptr + sizeof(AofHeader);
+            ref var key = ref Unsafe.AsRef<SpanByte>(curr);
+            curr += key.TotalSize;
+


This could be cleaned up with an AofRecordDescriptor or something like that, to cover these pointer manipulations

TedHartMS · 2024-10-04T17:13:53Z

libs/server/InputHeader.cs

        /// </summary>
-        public unsafe SpanByte SpanByte => new(Length, (nint)ToPointer());
+        public int parseStateStartIdx;



This is another opportunity for clearer naming: What index? into the byte*? The reader has to look to see it refers to arguments. You use "token" in some other places, which is also good but should be consistent. (I know this field name came from existing ObjectInput; it would be nice to make these all consistent and clear)

TedHartMS · 2024-10-05T00:15:32Z

libs/server/Resp/Bitmap/BitmapManagerBitPos.cs

-            long startOffset = *(long*)(input + sizeof(byte));
-            long endOffset = *(long*)(input + sizeof(byte) + sizeof(long));
-            byte offsetType = *(input + sizeof(byte) + sizeof(long) * 2);
-
            if (offsetType == 0x0)


offsetType should be an enum or at least named constants. Magic numbers are uninformative and less maintainable (I know this was there before, but since you're doing such extensive cleanup including this param, we should get this too)

TedHartMS · 2024-10-05T00:30:31Z

libs/server/Resp/Parser/ParseUtils.cs

+            return slice.length != 0 &&
+                   RespReadUtils.TryReadIntSafe(ref ptr, slice.ptr + slice.length, out number, out var bytesRead, out _,
+                       out _, false) &&
+                   (int)bytesRead == slice.length;


use "paramName: false" for readability

TedHartMS · 2024-10-05T00:50:55Z

libs/server/Storage/Functions/MainStore/PrivateMethods.cs

-                    E = HyperLogLog.DefaultHLL.Count(value.ToPointer());
-                    *(long*)dst.SpanByte.ToPointer() = E;
-                    return;
-


Was it intentional to remove the
E = HyperLogLog.DefaultHLL.Count(value.ToPointer());
call?

TedHartMS · 2024-10-05T01:28:52Z

libs/server/Storage/Session/MainStore/BitmapOps.cs

-            pcurr += sizeof(long);
-            *pcurr = (byte)(useBitInterval ? 1 : 0);
+            var startBytes = Encoding.ASCII.GetBytes(start.ToString(CultureInfo.InvariantCulture));
+            var endBytes = Encoding.ASCII.GetBytes(end.ToString(CultureInfo.InvariantCulture));


These do allocations. stackalloc long[1] instead?

TedHartMS · 2024-10-05T01:30:01Z

libs/server/Storage/Session/MainStore/BitmapOps.cs

-                *(long*)pcurr = commandArguments[i].value; pcurr += 8;
-                *pcurr = commandArguments[i].overflowType;
+                var op = (RespCommand)commandArguments[i].secondaryOpCode;
+                var opBytes = Encoding.ASCII.GetBytes(op.ToString());


More GetBytes allocations and also some ToString() allocations. Can these be avoided? If not, comment why they are necessary

TedHartMS · 2024-10-05T01:43:52Z

libs/server/Storage/Session/MainStore/HyperLogLogOps.cs

                {
-                    *(long*)pcurr = (long)HashUtils.MurmurHash2x64A(ptr, bString.Length);


It looks like the Murmur has moved into IterateUpdate. Can the string be fixed directly rather than another GetBytes?

badrishc and others added 30 commits June 13, 2024 15:49

partial checkin

5c570c9

nit

b29c03b

nit

8d5c67b

idea for initial spike for making input a struct type

57d9534

does not work

support ZADD and ZREM using safe struct wrappers for input

90c2489

nit

6ac7518

Merge branch 'main' into talzacc/header_impr

ce4e972

wip

76aa03d

wip

2698b67

Merge branch 'main' into talzacc/header_impr

b05050a

Fixing non-determinism + refactoring HashGet

fe19e3e

Merge branch 'talzacc/header_impr' of https://github.com/microsoft/ga…

5459083

…rnet into talzacc/header_impr

merging from latest main

34354be

dotnet format

cea36f6

Fixed some API calls

65a62c8

dotnet format

9132c87

merging with latest main

d4ea372

wip

9570d86

small fix

ea10ce6

Merge branch 'main' into talzacc/cmd_handling_impr

ba1f67c

Removing unused method

8d5a186

Merge branch 'talzacc/cmd_handling_impr' of https://github.com/micros…

ce208e3

…oft/garnet into talzacc/cmd_handling_impr

wip - refactoring ProcessAdminCommands

38fef93

Merging from main

abf8e28

Undoing changes to RandomUtils

925291d

Continued refactoring of AdminCommands

7a410c2

Added "TryGetInt" and "TryGetLong" to parse state api

6555787

dotnet format

6ee3652

wip

cadfca2

format

9df3e68

TalZaccai added 9 commits September 4, 2024 12:07

more fixes

979c9f9

merging with latest main

e2b93a6

Merge branch 'main' into talzacc/cmd_handling_impr4

a2e5e75

merging from main

7b5d5e4

fix

eaa00ab

Merge branch 'main' into talzacc/cmd_handling_impr4

b1309af

cleanup

ea1e337

cleanup

9fb4989

cleanup

12d4ff6

TalZaccai marked this pull request as ready for review September 5, 2024 02:25

TalZaccai requested review from badrishc, vazois and TedHartMS September 5, 2024 02:25

TalZaccai and others added 7 commits September 5, 2024 10:38

cleanup

0ec23cc

Merge from main + small bug fixes

626ddf0

Merge branch 'main' into talzacc/cmd_handling_impr4

4280997

expire bugfix

51a17ba

another bugfix

ca80161

small fix

7aa443e

Merge branch 'main' into talzacc/cmd_handling_impr4

344e5e7

vazois requested changes Sep 20, 2024

View reviewed changes

badrishc requested changes Sep 20, 2024

View reviewed changes

TalZaccai added 4 commits September 30, 2024 17:10

merging from latest main

f3a57fe

Fixing some comments

318bd36

removing some unnecessary allocations

b0891ce

fixes

ae0d1c5

badrishc mentioned this pull request Oct 4, 2024

[Compatibility] Added INCRBYFLOAT command #699

Open

3 tasks

format

2dbf562

TedHartMS requested changes Oct 4, 2024

View reviewed changes

TedHartMS requested changes Oct 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements in command handling #5 #631

Improvements in command handling #5 #631

TalZaccai commented Sep 3, 2024 •

edited

Loading

vazois Sep 20, 2024

TalZaccai Oct 1, 2024

vazois Sep 20, 2024

TalZaccai Oct 1, 2024

badrishc Oct 1, 2024

badrishc left a comment

TedHartMS left a comment

TedHartMS Oct 4, 2024

TedHartMS Oct 4, 2024

TedHartMS Oct 4, 2024

TedHartMS Oct 4, 2024

TedHartMS Oct 4, 2024

TedHartMS Oct 4, 2024

TedHartMS Oct 4, 2024

TedHartMS Oct 4, 2024

TedHartMS Oct 5, 2024

TedHartMS Oct 5, 2024

TedHartMS Oct 5, 2024

TedHartMS Oct 5, 2024

TedHartMS Oct 5, 2024

TedHartMS Oct 5, 2024

		{
		(long)pcurr = (long)HashUtils.MurmurHash2x64A(ptr, bString.Length);

Improvements in command handling #5 #631

Are you sure you want to change the base?

Improvements in command handling #5 #631

Conversation

TalZaccai commented Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badrishc left a comment

Choose a reason for hiding this comment

TedHartMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalZaccai commented Sep 3, 2024 •

edited

Loading