Eliminating usage of Enum.Parse/Enum.TryParse from critical performan…

…ce paths (#806) * Eliminating usage of Enum.Parse/TryParse from critical performance paths * wip * wip * bugfix * format * Fixing comment * Moving parsing logic to SessionParseState extension methods * Removing unnecessary usings * format * Added 2 parsers
microsoft · Nov 18, 2024 · fbc6b41 · fbc6b41
1 parent 82bf21e
commit fbc6b41
Show file tree

Hide file tree

Showing 20 changed files with 382 additions and 214 deletions.
diff --git a/libs/cluster/Server/HashSlot.cs b/libs/cluster/Server/HashSlot.cs
@@ -1,7 +1,6 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-using System;
 using System.Runtime.InteropServices;
 
 namespace Garnet.cluster
@@ -11,6 +10,8 @@ namespace Garnet.cluster
     /// </summary>
     public enum SlotState : byte
     {
+        // IMPORTANT: Any changes to the values of this enum should be reflected in its parser (SessionParseStateExtensions.TryGetSlotState)
+
         /// <summary>
         /// Slot not assigned
         /// </summary>
@@ -41,23 +42,6 @@ public enum SlotState : byte
         INVALID,
     }
 
-    /// <summary>
-    /// Extension methods for <see cref="SlotState"/>.
-    /// </summary>
-    public static class SlotStateExtensions
-    {
-        /// <summary>
-        /// Validate that the given <see cref="SlotState"/> is legal, and _could_ have come from the given <see cref="ReadOnlySpan{T}"/>.
-        /// 
-        /// TODO: Long term we can kill this and use <see cref="IUtf8SpanParsable{ClientType}"/> instead of <see cref="Enum.TryParse{TEnum}(string?, bool, out TEnum)"/>
-        /// and avoid extra validation.  See: https://github.com/dotnet/runtime/issues/81500 .
-        /// </summary>
-        public static bool IsValid(this SlotState type, ReadOnlySpan<byte> fromSpan)
-        {
-            return type != SlotState.INVALID && type != SlotState.OFFLINE && Enum.IsDefined(type) && !fromSpan.ContainsAnyInRange((byte)'0', (byte)'9');
-        }
-    }
-
     /// <summary>
     /// Hashslot info
     /// </summary>

diff --git a/libs/cluster/Session/FailoverCommand.cs b/libs/cluster/Session/FailoverCommand.cs
@@ -21,7 +21,8 @@ private bool TryFAILOVER()
             var currTokenIdx = 0;
             while (currTokenIdx < parseState.Count)
             {
-                if (!parseState.TryGetEnum(currTokenIdx, true, out FailoverOption failoverOption) || !failoverOption.IsValid(parseState.GetArgSliceByRef(currTokenIdx).ReadOnlySpan))
+                if (!parseState.TryGetFailoverOption(currTokenIdx, out var failoverOption) ||
+                    failoverOption == FailoverOption.DEFAULT || failoverOption == FailoverOption.INVALID)
                 {
                     while (!RespWriteUtils.WriteError(CmdStrings.RESP_SYNTAX_ERROR, ref dcurr, dend))
                         SendAndReset();

diff --git a/libs/cluster/Session/RespClusterFailoverCommands.cs b/libs/cluster/Session/RespClusterFailoverCommands.cs
@@ -30,7 +30,8 @@ private bool NetworkClusterFailover(out bool invalidParameters)
             if (parseState.Count > 0)
             {
                 // Try to parse failover option
-                if (!parseState.TryGetEnum(0, ignoreCase: true, out failoverOption) || !failoverOption.IsValid(parseState.GetArgSliceByRef(0).Span))
+                if (!parseState.TryGetFailoverOption(0, out failoverOption) ||
+                    failoverOption == FailoverOption.DEFAULT || failoverOption == FailoverOption.INVALID)
                 {
                     var failoverOptionStr = parseState.GetString(0);
 

diff --git a/libs/cluster/Session/RespClusterSlotManagementCommands.cs b/libs/cluster/Session/RespClusterSlotManagementCommands.cs
@@ -439,7 +439,8 @@ private bool NetworkClusterSetSlot(out bool invalidParameters)
                 return true;
             }
 
-            if (!parseState.TryGetEnum<SlotState>(1, ignoreCase: true, out var slotState) || !slotState.IsValid(parseState.GetArgSliceByRef(1).ReadOnlySpan))
+            if (!parseState.TryGetSlotState(1, out var slotState) || slotState == SlotState.INVALID ||
+                slotState == SlotState.OFFLINE)
             {
                 var slotStateStr = parseState.GetString(1);
                 while (!RespWriteUtils.WriteError($"ERR Slot state {slotStateStr} not supported.", ref dcurr, dend))
@@ -531,13 +532,11 @@ private bool NetworkClusterSetSlotsRange(out bool invalidParameters)
             // CLUSTER SETSLOTRANGE STABLE <slot-start> <slot-end> [slot-start slot-end]
             string nodeId = default;
 
-            // Extract subcommand
-            var subcommand = parseState.GetString(0);
-
             // Try parse slot state
-            if (!Enum.TryParse(subcommand, ignoreCase: true, out SlotState slotState))
+            if (!parseState.TryGetSlotState(0, out var slotState))
             {
                 // Log error for invalid slot state option
+                var subcommand = parseState.GetString(0);
                 logger?.LogError("The given input '{input}' is not a valid slot state option.", subcommand);
                 slotState = SlotState.INVALID;
             }
@@ -583,6 +582,7 @@ private bool NetworkClusterSetSlotsRange(out bool invalidParameters)
                     break;
                 default:
                     setSlotsSucceeded = false;
+                    var subcommand = parseState.GetString(0);
                     errorMessage = Encoding.ASCII.GetBytes($"ERR Slot state {subcommand} not supported.");
                     break;
             }

diff --git a/libs/cluster/SessionParseStateExtensions.cs b/libs/cluster/SessionParseStateExtensions.cs
@@ -0,0 +1,76 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+using Garnet.common;
+using Garnet.server;
+
+namespace Garnet.cluster
+{
+    /// <summary>
+    /// Extension methods for <see cref="SessionParseState"/>.
+    /// </summary>
+    public static class SessionParseStateExtensions
+    {
+        /// <summary>
+        /// Parse slot state from parse state at specified index
+        /// </summary>
+        /// <param name="parseState">The parse state</param>
+        /// <param name="idx">The argument index</param>
+        /// <param name="value">Parsed value</param>
+        /// <returns>True if value parsed successfully</returns>
+        public static bool TryGetSlotState(this SessionParseState parseState, int idx, out SlotState value)
+        {
+            value = default;
+            var sbArg = parseState.GetArgSliceByRef(idx).ReadOnlySpan;
+
+            if (sbArg.EqualsUpperCaseSpanIgnoringCase("OFFLINE"u8))
+                value = SlotState.OFFLINE;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("STABLE"u8))
+                value = SlotState.STABLE;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("MIGRATING"u8))
+                value = SlotState.MIGRATING;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("IMPORTING"u8))
+                value = SlotState.IMPORTING;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("FAIL"u8))
+                value = SlotState.FAIL;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("NODE"u8))
+                value = SlotState.NODE;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("INVALID"u8))
+                value = SlotState.INVALID;
+            else return false;
+
+            return true;
+        }
+
+        /// <summary>
+        /// Parse failover option from parse state at specified index
+        /// </summary>
+        /// <param name="parseState">The parse state</param>
+        /// <param name="idx">The argument index</param>
+        /// <param name="value">Parsed value</param>
+        /// <returns>True if value parsed successfully</returns>
+        public static bool TryGetFailoverOption(this SessionParseState parseState, int idx, out FailoverOption value)
+        {
+            value = default;
+            var sbArg = parseState.GetArgSliceByRef(idx).ReadOnlySpan;
+
+            if (sbArg.EqualsUpperCaseSpanIgnoringCase("DEFAULT"u8))
+                value = FailoverOption.DEFAULT;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("INVALID"u8))
+                value = FailoverOption.INVALID;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("TO"u8))
+                value = FailoverOption.TO;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("FORCE"u8))
+                value = FailoverOption.FORCE;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("ABORT"u8))
+                value = FailoverOption.ABORT;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("TIMEOUT"u8))
+                value = FailoverOption.TIMEOUT;
+            else if (sbArg.EqualsUpperCaseSpanIgnoringCase("TAKEOVER"u8))
+                value = FailoverOption.TAKEOVER;
+            else return false;
+
+            return true;
+        }
+    }
+}
diff --git a/libs/common/FailoverOption.cs b/libs/common/FailoverOption.cs
@@ -12,6 +12,8 @@ namespace Garnet.common
     /// </summary>
     public enum FailoverOption : byte
     {
+        // IMPORTANT: Any changes to the values of this enum should be reflected in its parser (SessionParseStateExtensions.TryGetFailoverOption)
+
         /// <summary>
         /// Internal use only
         /// </summary>
@@ -59,21 +61,4 @@ public static class FailoverUtils
         public static byte[] GetRespFormattedFailoverOption(FailoverOption failoverOption)
             => infoSections[(int)failoverOption];
     }
-
-    /// <summary>
-    /// Extension methods for <see cref="FailoverOption"/>.
-    /// </summary>
-    public static class FailoverOptionExtensions
-    {
-        /// <summary>
-        /// Validate that the given <see cref="FailoverOption"/> is legal, and _could_ have come from the given <see cref="ReadOnlySpan{T}"/>.
-        /// 
-        /// TODO: Long term we can kill this and use <see cref="IUtf8SpanParsable{ClientType}"/> instead of <see cref="Enum.TryParse{TEnum}(string?, bool, out TEnum)"/>
-        /// and avoid extra validation.  See: https://github.com/dotnet/runtime/issues/81500 .
-        /// </summary>
-        public static bool IsValid(this FailoverOption type, ReadOnlySpan<byte> fromSpan)
-        {
-            return type != FailoverOption.DEFAULT && type != FailoverOption.INVALID && Enum.IsDefined(type) && !fromSpan.ContainsAnyInRange((byte)'0', (byte)'9');
-        }
-    }
 }
diff --git a/libs/common/Metrics/InfoMetricsType.cs b/libs/common/Metrics/InfoMetricsType.cs
@@ -12,6 +12,8 @@ namespace Garnet.common
     /// </summary>
     public enum InfoMetricsType : byte
     {
+        // IMPORTANT: Any changes to the values of this enum should be reflected in its parser (SessionParseStateExtensions.TryGetInfoMetricsType)
+
         /// <summary>
         /// Server info
         /// </summary>

diff --git a/libs/common/Metrics/LatencyMetricsType.cs b/libs/common/Metrics/LatencyMetricsType.cs
@@ -8,6 +8,8 @@ namespace Garnet.common
     /// </summary>
     public enum LatencyMetricsType : byte
     {
+        // IMPORTANT: Any changes to the values of this enum should be reflected in its parser (SessionParseStateExtensions.TryGetLatencyMetricsType)
+
         /// <summary>
         /// Latency of processing, per network receive call (server side) - consider batches with only non-admin requests
         /// Measured from when we start processing the first request in packet, until when we finishing processing the last request in packet (including sending responses).

diff --git a/libs/server/ClientType.cs b/libs/server/ClientType.cs
@@ -1,15 +1,15 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-using System;
-
 namespace Garnet.server
 {
     /// <summary>
     /// Type option for CLIENT|LIST and CLIENT|KILL commands.
     /// </summary>
     public enum ClientType : byte
     {
+        // IMPORTANT: Any changes to the values of this enum should be reflected in its parser (SessionParseStateExtensions.TryGetClientType)
+
         /// <summary>
         /// Default invalid case.
         /// </summary>
@@ -39,18 +39,4 @@ public enum ClientType : byte
         /// </summary>
         SLAVE,
     }
-
-    public static class ClientTypeExtensions
-    {
-        /// <summary>
-        /// Validate that the given <see cref="ClientType"/> is legal, and _could_ have come from the given <see cref="ArgSlice"/>.
-        /// 
-        /// TODO: Long term we can kill this and use <see cref="IUtf8SpanParsable{ClientType}"/> instead of <see cref="Enum.TryParse{TEnum}(string?, bool, out TEnum)"/>
-        /// and avoid extra validation.  See: https://github.com/dotnet/runtime/issues/81500 .
-        /// </summary>
-        public static bool IsValid(this ClientType type, ref ArgSlice fromSlice)
-        {
-            return type != ClientType.Invalid && Enum.IsDefined(type) && !fromSlice.ReadOnlySpan.ContainsAnyInRange((byte)'0', (byte)'9');
-        }
-    }
 }
diff --git a/libs/server/Metrics/Info/InfoCommand.cs b/libs/server/Metrics/Info/InfoCommand.cs
@@ -20,31 +20,25 @@ private bool NetworkINFO()
             if (count > 0)
             {
                 sections = new HashSet<InfoMetricsType>();
-                for (int i = 0; i < count; i++)
+                for (var i = 0; i < count; i++)
                 {
-                    var section = parseState.GetString(i).ToUpper();
+                    var sbSection = parseState.GetArgSliceByRef(i).ReadOnlySpan;
 
-                    switch (section)
+                    if (sbSection.EqualsUpperCaseSpanIgnoringCase("RESET"u8))
+                        reset = true;
+                    else if (sbSection.EqualsUpperCaseSpanIgnoringCase("HELP"u8))
+                        help = true;
+                    else if (!sbSection.EqualsUpperCaseSpanIgnoringCase("ALL"u8))
                     {
-                        case InfoHelp.RESET:
-                            reset = true;
-                            break;
-                        case InfoHelp.HELP:
-                            help = true;
-                            break;
-                        case InfoHelp.ALL:
-                            break;
-                        default:
-                            if (Enum.TryParse(section, out InfoMetricsType sectionType))
-                            {
-                                sections.Add(sectionType);
-                            }
-                            else
-                            {
-                                invalid = true;
-                                invalidSection = section;
-                            }
-                            break;
+                        if (parseState.TryGetInfoMetricsType(i, out var sectionType))
+                        {
+                            sections.Add(sectionType);
+                        }
+                        else
+                        {
+                            invalid = true;
+                            invalidSection = parseState.GetString(i);
+                        }
                     }
                 }
             }

diff --git a/libs/server/Metrics/Latency/RespLatencyCommands.cs b/libs/server/Metrics/Latency/RespLatencyCommands.cs
@@ -50,16 +50,14 @@ private bool NetworkLatencyHistogram()
                 events = new();
                 for (int i = 0; i < parseState.Count; i++)
                 {
-                    var eventStr = parseState.GetString(i);
-
-                    if (Enum.TryParse(eventStr, ignoreCase: true, out LatencyMetricsType eventType))
+                    if (parseState.TryGetLatencyMetricsType(i, out var eventType))
                     {
                         events.Add(eventType);
                     }
                     else
                     {
                         invalid = true;
-                        invalidEvent = eventStr;
+                        invalidEvent = parseState.GetString(i);
                     }
                 }
             }
@@ -98,16 +96,14 @@ private bool NetworkLatencyReset()
                 events = new();
                 for (int i = 0; i < parseState.Count; i++)
                 {
-                    var eventStr = parseState.GetString(i);
-
-                    if (Enum.TryParse(eventStr, ignoreCase: true, out LatencyMetricsType eventType))
+                    if (parseState.TryGetLatencyMetricsType(i, out var eventType))
                     {
                         events.Add(eventType);
                     }
                     else
                     {
                         invalid = true;
-                        invalidEvent = eventStr;
+                        invalidEvent = parseState.GetString(i);
                     }
                 }
             }

diff --git a/libs/server/Objects/SortedSet/SortedSetObjectImpl.cs b/libs/server/Objects/SortedSet/SortedSetObjectImpl.cs
@@ -30,49 +30,13 @@ private struct ZRangeOptions
             public bool WithScores { get; set; }
         };
 
-        bool TryGetSortedSetAddOption(ReadOnlySpan<byte> item, out SortedSetAddOption options)
-        {
-            if (item.EqualsUpperCaseSpanIgnoringCase("XX"u8))
-            {
-                options = SortedSetAddOption.XX;
-                return true;
-            }
-            if (item.EqualsUpperCaseSpanIgnoringCase("NX"u8))
-            {
-                options = SortedSetAddOption.NX;
-                return true;
-            }
-            if (item.EqualsUpperCaseSpanIgnoringCase("LT"u8))
-            {
-                options = SortedSetAddOption.LT;
-                return true;
-            }
-            if (item.EqualsUpperCaseSpanIgnoringCase("GT"u8))
-            {
-                options = SortedSetAddOption.GT;
-                return true;
-            }
-            if (item.EqualsUpperCaseSpanIgnoringCase("CH"u8))
-            {
-                options = SortedSetAddOption.CH;
-                return true;
-            }
-            if (item.EqualsUpperCaseSpanIgnoringCase("INCR"u8))
-            {
-                options = SortedSetAddOption.INCR;
-                return true;
-            }
-            options = SortedSetAddOption.None;
-            return false;
-        }
-
         bool GetOptions(ref ObjectInput input, ref int currTokenIdx, out SortedSetAddOption options, ref byte* curr, byte* end, ref SpanByteAndMemory output, ref bool isMemory, ref byte* ptr, ref MemoryHandle ptrHandle)
         {
             options = SortedSetAddOption.None;
 
             while (currTokenIdx < input.parseState.Count)
             {
-                if (!TryGetSortedSetAddOption(input.parseState.GetArgSliceByRef(currTokenIdx).ReadOnlySpan, out var currOption))
+                if (!input.parseState.TryGetSortedSetAddOption(currTokenIdx, out var currOption))
                     break;
 
                 options |= currOption;