Increment/Decrement Valid Number Check (#262)

* check for valid number * wip * added IncrErrorType * added test cases & include fix for leading zero validity check * formatting * more formatting * addressing review comments * Check overflow on number load and remove benchmark * added overflow check and copyupdater validity check * first attempt bdn with embedded bench * use TryBytesLong on InitialLength calculcations * add more unit tests * fix initial length parse check
microsoft · Apr 16, 2024 · a8820b3 · a8820b3
1 parent e382d3b
commit a8820b3
Show file tree

Hide file tree

Showing 26 changed files with 443 additions and 66 deletions.
diff --git a/benchmark/BDN.benchmark/BDN.benchmark.csproj b/benchmark/BDN.benchmark/BDN.benchmark.csproj
@@ -5,12 +5,20 @@
     <ImplicitUsings>enable</ImplicitUsings>
 		<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
 		<LangVersion>latest</LangVersion>
-    <Nullable>enable</Nullable>
+		<SignAssembly>true</SignAssembly>
+		<AssemblyOriginatorKeyFile>../../Garnet.snk</AssemblyOriginatorKeyFile>
+		<DelaySign>false</DelaySign>
   </PropertyGroup>
 	<ItemGroup>
 		<PackageReference Include="BenchmarkDotNet" Version="0.13.12" />
 	</ItemGroup>
 	<ItemGroup>
+		<ProjectReference Include="..\..\libs\host\Garnet.host.csproj" />
 		<ProjectReference Include="..\..\libs\common\Garnet.common.csproj" />
+		<ProjectReference Include="..\..\libs\server\Garnet.server.csproj" />
+	</ItemGroup>
+	<ItemGroup>
+		<Compile Include="..\..\playground\Embedded.perftest\EmbeddedRespServer.cs" Link="EmbeddedRespServer.cs" />
+		<Compile Include="..\..\playground\Embedded.perftest\DummyNetworkSender.cs" Link="DummyNetworkSender.cs" />
 	</ItemGroup>
 </Project>
diff --git a/benchmark/BDN.benchmark/Resp/RespIncrementStress.cs b/benchmark/BDN.benchmark/Resp/RespIncrementStress.cs
@@ -0,0 +1,92 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+using System.Text;
+using BenchmarkDotNet.Attributes;
+using Embedded.perftest;
+using Garnet.common;
+using Garnet.server;
+
+namespace BDN.benchmark.Resp
+{
+    public unsafe class RespIncrementStress
+    {
+        EmbeddedRespServer server;
+        RespServerSession session;
+
+        static ReadOnlySpan<byte> SINGLE_INCR => "*2\r\n$4\r\nINCR\r\n$1\r\nx\r\n"u8;
+        static ReadOnlySpan<byte> SINGLE_DECR => "*2\r\n$4\r\nDECR\r\n$1\r\nx\r\n"u8;
+
+        [GlobalSetup]
+        public void GlobalSetup()
+        {
+            var opt = new GarnetServerOptions
+            {
+                QuietMode = true
+            };
+            server = new EmbeddedRespServer(opt);
+            session = server.GetRespSession();
+        }
+
+        [GlobalCleanup]
+        public void GlobalCleanup()
+        {
+            session.Dispose();
+            server.Dispose();
+        }
+
+        [Benchmark]
+        public void Increment()
+        {
+            fixed (byte* ptr = SINGLE_INCR)
+            {
+                _ = session.TryConsumeMessages(ptr, SINGLE_INCR.Length);
+            }
+        }
+
+        [Benchmark]
+        public void Decrement()
+        {
+            fixed (byte* ptr = SINGLE_DECR)
+            {
+                _ = session.TryConsumeMessages(ptr, SINGLE_DECR.Length);
+            }
+        }
+
+        [Benchmark]
+        [ArgumentsSource(nameof(IncrByBenchInput))]
+        public void IncrementBy(byte[] input)
+        {
+            fixed (byte* ptr = input)
+            {
+                _ = session.TryConsumeMessages(ptr, input.Length);
+            }
+        }
+
+        [Benchmark]
+        [ArgumentsSource(nameof(DecrByBenchInput))]
+        public void DecrementBy(byte[] input)
+        {
+            fixed (byte* ptr = input)
+            {
+                _ = session.TryConsumeMessages(ptr, input.Length);
+            }
+        }
+
+        public static IEnumerable<object> IncrByBenchInput => values.Select(x => Get(RespCommand.INCRBY, x));
+        public static IEnumerable<object> DecrByBenchInput => values.Select(x => Get(RespCommand.DECRBY, x));
+
+        public static int[] values => [int.MinValue, -1, 0, int.MaxValue];
+
+        public static byte[] Get(RespCommand cmd, long val)
+        {
+            var length = NumUtils.NumDigitsInLong(val);
+            return cmd switch
+            {
+                RespCommand.INCRBY => Encoding.ASCII.GetBytes($"*2\r\n$6\r\nINCRBY\r\n$1\r\nx\r\n${length}\r\n{val}\r\n"),
+                RespCommand.DECRBY => Encoding.ASCII.GetBytes($"*2\r\n$6\r\nDECRBY\r\n$1\r\nx\r\n${length}\r\n{val}\r\n"),
+                _ => throw new Exception($"Option {cmd} not supported")
+            };
+        }
+    }
+}
diff --git a/benchmark/Resp.benchmark/RespOnlineBench.cs b/benchmark/Resp.benchmark/RespOnlineBench.cs
@@ -6,7 +6,6 @@
 using System.Diagnostics;
 using System.Linq;
 using System.Runtime.CompilerServices;
-using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using Garnet.client;

diff --git a/libs/client/ClientSession/GarnetClientSessionReplicationExtensions.cs b/libs/client/ClientSession/GarnetClientSessionReplicationExtensions.cs
@@ -2,7 +2,6 @@
 // Licensed under the MIT license.
 
 using System;
-using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using Garnet.common;

diff --git a/libs/common/NumUtils.cs b/libs/common/NumUtils.cs
@@ -13,6 +13,8 @@ namespace Garnet.common
     /// </summary>
     public static unsafe class NumUtils
     {
+        public const int MaximumFormatInt64Length = 20;   // 19 + sign (i.e. -9223372036854775808)
+
         /// <summary>
         /// Convert long number into sequence of ASCII bytes
         /// </summary>
@@ -102,6 +104,36 @@ public static long BytesToLong(int length, byte* source)
             return fNeg ? -(result) : result;
         }
 
+        /// <summary>
+        /// Convert sequence of ASCII bytes into long number
+        /// </summary>
+        /// <param name="length">Length of number</param>
+        /// <param name="source">Source bytes</param>
+        /// <param name="result">Long value extracted from sequence</param>
+        /// <returns>True if sequence contains only numeric digits, otherwise false</returns>
+        public static bool TryBytesToLong(int length, byte* source, out long result)
+        {
+            var fNeg = *source == '-';
+            var beg = fNeg ? source + 1 : source;
+            var end = source + length;
+            var digit = *beg - '0';
+            result = 0;
+
+            // Check first digit which needs to be non-zero
+            if (digit is <= 0 or > 9)
+                return false;
+
+            while (beg < end)
+            {
+                digit = *beg++ - '0';
+                if (digit is < 0 or > 9)
+                    return false;
+                checked { result = (result * 10) + digit; }
+            }
+            result = fNeg ? -result : result;
+            return true;
+        }
+
         /// <summary>
         /// Convert sequence of ASCII bytes into ulong number
         /// </summary>
@@ -512,6 +544,5 @@ public static unsafe bool TryBytesToInt(byte* source, int len, out int result)
             result = fNeg ? -(result) : result;
             return true;
         }
-
     }
 }
diff --git a/libs/server/Custom/CustomRespCommands.cs b/libs/server/Custom/CustomRespCommands.cs
@@ -4,7 +4,6 @@
 using System;
 using System.Diagnostics;
 using System.Runtime.CompilerServices;
-using System.Text;
 using Garnet.common;
 using Tsavorite.core;
 

diff --git a/libs/server/Metrics/Info/InfoCommand.cs b/libs/server/Metrics/Info/InfoCommand.cs
@@ -4,7 +4,6 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
-using System.Text;
 using Garnet.common;
 
 namespace Garnet.server

diff --git a/libs/server/OperationError.cs b/libs/server/OperationError.cs
@@ -0,0 +1,20 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+namespace Garnet.server
+{
+    /// <summary>
+    /// Operation error type
+    /// </summary>
+    public enum OperationError : byte
+    {
+        /// <summary>
+        /// Operation on data type succeeded
+        /// </summary>
+        SUCCESS,
+        /// <summary>
+        /// Operation failed due to incompatible type
+        /// </summary>
+        INVALID_TYPE
+    }
+}
diff --git a/libs/server/Properties/AssemblyInfo.cs b/libs/server/Properties/AssemblyInfo.cs
@@ -5,6 +5,7 @@
 
 [assembly: InternalsVisibleTo("Garnet.test" + AssemblyRef.GarnetPublicKey)]
 [assembly: InternalsVisibleTo("Embedded.perftest" + AssemblyRef.GarnetPublicKey)]
+[assembly: InternalsVisibleTo("BDN.benchmark" + AssemblyRef.GarnetPublicKey)]
 
 /// <summary>
 /// Sets public key string for friend assemblies.

diff --git a/libs/server/Resp/BasicCommands.cs b/libs/server/Resp/BasicCommands.cs
@@ -770,15 +770,25 @@ private bool NetworkIncrement<TGarnetApi>(byte* ptr, RespCommand cmd, ref TGarne
                 return true;
 
             var key = new ArgSlice(keyPtr, ksize);
-
-            byte* pbOutput = stackalloc byte[20];
-            var output = new ArgSlice(pbOutput, 20);
+            var pbOutput = stackalloc byte[NumUtils.MaximumFormatInt64Length + 1];
+            var output = new ArgSlice(pbOutput, NumUtils.MaximumFormatInt64Length + 1);
 
             var status = storageApi.Increment(key, input, ref output);
+            var errorFlag = output.Length == NumUtils.MaximumFormatInt64Length + 1 ? (OperationError)output.Span[0] : OperationError.SUCCESS;
 
-            while (!RespWriteUtils.WriteIntegerFromBytes(pbOutput, output.Length, ref dcurr, dend))
-                SendAndReset();
-
+            switch (errorFlag)
+            {
+                case OperationError.SUCCESS:
+                    while (!RespWriteUtils.WriteIntegerFromBytes(pbOutput, output.Length, ref dcurr, dend))
+                        SendAndReset();
+                    break;
+                case OperationError.INVALID_TYPE:
+                    while (!RespWriteUtils.WriteError(CmdStrings.RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER, ref dcurr, dend))
+                        SendAndReset();
+                    break;
+                default:
+                    throw new GarnetException($"Invalid OperationError {errorFlag}");
+            }
             return true;
         }
 

diff --git a/libs/server/Resp/Bitmap/BitmapCommands.cs b/libs/server/Resp/Bitmap/BitmapCommands.cs
@@ -6,7 +6,6 @@
 using System.Diagnostics;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
-using System.Text;
 using Garnet.common;
 using Microsoft.Extensions.Logging;
 using Tsavorite.core;

diff --git a/libs/server/Resp/CmdStrings.cs b/libs/server/Resp/CmdStrings.cs
@@ -123,6 +123,7 @@ public static ReadOnlySpan<byte> GetConfig(ReadOnlySpan<byte> key)
         public static ReadOnlySpan<byte> RESP_ERR_GENERIC_REGISTERCS_UNSUPPORTED_CLASS => "ERR unable to register one or more unsupported classes."u8;
         public static ReadOnlySpan<byte> RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER => "ERR value is not an integer or out of range."u8;
         public static ReadOnlySpan<byte> RESP_ERR_GENERIC_UKNOWN_SUBCOMMAND => "ERR Unknown subcommand. Try LATENCY HELP."u8;
+
         /// <summary>
         /// Response string templates
         /// </summary>

diff --git a/libs/server/Resp/KeyAdminCommands.cs b/libs/server/Resp/KeyAdminCommands.cs
@@ -4,7 +4,6 @@
 using System;
 using System.Diagnostics;
 using System.Runtime.CompilerServices;
-using System.Text;
 using Garnet.common;
 using Tsavorite.core;
 

diff --git a/libs/server/Resp/Objects/ListCommands.cs b/libs/server/Resp/Objects/ListCommands.cs
@@ -3,7 +3,6 @@
 
 using System;
 using System.Linq;
-using System.Text;
 using Garnet.common;
 using Tsavorite.core;
 

diff --git a/libs/server/Resp/Objects/SortedSetCommands.cs b/libs/server/Resp/Objects/SortedSetCommands.cs
@@ -2,7 +2,6 @@
 // Licensed under the MIT license.
 
 using System;
-using System.Text;
 using Garnet.common;
 using Tsavorite.core;
 

diff --git a/libs/server/Resp/PubSubCommands.cs b/libs/server/Resp/PubSubCommands.cs
@@ -4,7 +4,6 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
-using System.Text;
 using Garnet.common;
 using Tsavorite.core;