Skip to content

Commit bc8c7c0

Browse files
badrishcbadrishc
authored
Reduce use of low-perf TryParse for enum in codebase (#693)
* Reduce use of low-perf TryParse for enum in codebase * another improvement to avoid enum parsing in ZADD * fix
1 parent eb40cd9 commit bc8c7c0

File tree

3 files changed

+133
-67
lines changed

3 files changed

+133
-67
lines changed

‎libs/server/ExpireOption.cs

-19
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT license.
33

4-
using System;
5-
64
namespace Garnet.server
75
{
86
/// <summary>
@@ -31,21 +29,4 @@ public enum ExpireOption : byte
3129
/// </summary>
3230
LT
3331
}
34-
35-
/// <summary>
36-
/// Extension methods for <see cref="ExpireOption"/>.
37-
/// </summary>
38-
public static class ExpireOptionExtensions
39-
{
40-
/// <summary>
41-
/// Validate that the given <see cref="ExpireOption"/> is legal, and _could_ have come from the given <see cref="ArgSlice"/>.
42-
///
43-
/// TODO: Long term we can kill this and use <see cref="IUtf8SpanParsable{ClientType}"/> instead of <see cref="Enum.TryParse{TEnum}(string?, bool, out TEnum)"/>
44-
/// and avoid extra validation. See: https://github.com/dotnet/runtime/issues/81500 .
45-
/// </summary>
46-
public static bool IsValid(this ExpireOption type, ref ArgSlice fromSlice)
47-
{
48-
return type != ExpireOption.None && Enum.IsDefined(type) && !fromSlice.ReadOnlySpan.ContainsAnyInRange((byte)'0', (byte)'9');
49-
}
50-
}
5132
}

‎libs/server/Objects/SortedSet/SortedSetObjectImpl.cs

+106-47
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,91 @@ private struct ZRangeOptions
3030
public bool WithScores { get; set; }
3131
};
3232

33+
bool TryGetSortedSetAddOption(ReadOnlySpan<byte> item, out SortedSetAddOption options)
34+
{
35+
if (item.EqualsUpperCaseSpanIgnoringCase("XX"u8))
36+
{
37+
options = SortedSetAddOption.XX;
38+
return true;
39+
}
40+
if (item.EqualsUpperCaseSpanIgnoringCase("NX"u8))
41+
{
42+
options = SortedSetAddOption.NX;
43+
return true;
44+
}
45+
if (item.EqualsUpperCaseSpanIgnoringCase("LT"u8))
46+
{
47+
options = SortedSetAddOption.LT;
48+
return true;
49+
}
50+
if (item.EqualsUpperCaseSpanIgnoringCase("GT"u8))
51+
{
52+
options = SortedSetAddOption.GT;
53+
return true;
54+
}
55+
if (item.EqualsUpperCaseSpanIgnoringCase("CH"u8))
56+
{
57+
options = SortedSetAddOption.CH;
58+
return true;
59+
}
60+
if (item.EqualsUpperCaseSpanIgnoringCase("INCR"u8))
61+
{
62+
options = SortedSetAddOption.INCR;
63+
return true;
64+
}
65+
options = SortedSetAddOption.None;
66+
return false;
67+
}
68+
69+
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)
70+
{
71+
options = SortedSetAddOption.None;
72+
73+
while (currTokenIdx < input.parseState.Count)
74+
{
75+
if (!TryGetSortedSetAddOption(input.parseState.GetArgSliceByRef(currTokenIdx).ReadOnlySpan, out var currOption))
76+
break;
77+
78+
options |= currOption;
79+
currTokenIdx++;
80+
}
81+
82+
// Validate ZADD options combination
83+
ReadOnlySpan<byte> optionsError = default;
84+
85+
// XX & NX are mutually exclusive
86+
if (options.HasFlag(SortedSetAddOption.XX) && options.HasFlag(SortedSetAddOption.NX))
87+
optionsError = CmdStrings.RESP_ERR_XX_NX_NOT_COMPATIBLE;
88+
89+
// NX, GT & LT are mutually exclusive
90+
if ((options.HasFlag(SortedSetAddOption.GT) && options.HasFlag(SortedSetAddOption.LT)) ||
91+
((options.HasFlag(SortedSetAddOption.GT) || options.HasFlag(SortedSetAddOption.LT)) &&
92+
options.HasFlag(SortedSetAddOption.NX)))
93+
optionsError = CmdStrings.RESP_ERR_GT_LT_NX_NOT_COMPATIBLE;
94+
95+
// INCR supports only one score-element pair
96+
if (options.HasFlag(SortedSetAddOption.INCR) && (input.parseState.Count - currTokenIdx > 2))
97+
optionsError = CmdStrings.RESP_ERR_INCR_SUPPORTS_ONLY_SINGLE_PAIR;
98+
99+
if (!optionsError.IsEmpty)
100+
{
101+
while (!RespWriteUtils.WriteError(optionsError, ref curr, end))
102+
ObjectUtils.ReallocateOutput(ref output, ref isMemory, ref ptr, ref ptrHandle, ref curr, ref end);
103+
return false;
104+
}
105+
106+
// From here on we expect only score-element pairs
107+
// Remaining token count should be positive and even
108+
if (currTokenIdx == input.parseState.Count || (input.parseState.Count - currTokenIdx) % 2 != 0)
109+
{
110+
while (!RespWriteUtils.WriteError(CmdStrings.RESP_SYNTAX_ERROR, ref curr, end))
111+
ObjectUtils.ReallocateOutput(ref output, ref isMemory, ref ptr, ref ptrHandle, ref curr, ref end);
112+
return false;
113+
}
114+
115+
return true;
116+
}
117+
33118
private void SortedSetAdd(ref ObjectInput input, ref SpanByteAndMemory output)
34119
{
35120
var isMemory = false;
@@ -46,60 +131,34 @@ private void SortedSetAdd(ref ObjectInput input, ref SpanByteAndMemory output)
46131
try
47132
{
48133
var options = SortedSetAddOption.None;
49-
50134
var currTokenIdx = input.parseStateStartIdx;
51-
while (currTokenIdx < input.parseState.Count)
52-
{
53-
if (!input.parseState.TryGetEnum(currTokenIdx, true, out SortedSetAddOption currOption))
54-
break;
55-
56-
options |= currOption;
57-
currTokenIdx++;
58-
}
59-
60-
// Validate ZADD options combination
61-
ReadOnlySpan<byte> optionsError = default;
62-
63-
// XX & NX are mutually exclusive
64-
if (options.HasFlag(SortedSetAddOption.XX) && options.HasFlag(SortedSetAddOption.NX))
65-
optionsError = CmdStrings.RESP_ERR_XX_NX_NOT_COMPATIBLE;
66-
67-
// NX, GT & LT are mutually exclusive
68-
if ((options.HasFlag(SortedSetAddOption.GT) && options.HasFlag(SortedSetAddOption.LT)) ||
69-
((options.HasFlag(SortedSetAddOption.GT) || options.HasFlag(SortedSetAddOption.LT)) &&
70-
options.HasFlag(SortedSetAddOption.NX)))
71-
optionsError = CmdStrings.RESP_ERR_GT_LT_NX_NOT_COMPATIBLE;
72-
73-
// INCR supports only one score-element pair
74-
if (options.HasFlag(SortedSetAddOption.INCR) && (input.parseState.Count - currTokenIdx > 2))
75-
optionsError = CmdStrings.RESP_ERR_INCR_SUPPORTS_ONLY_SINGLE_PAIR;
76-
77-
if (!optionsError.IsEmpty)
78-
{
79-
while (!RespWriteUtils.WriteError(optionsError, ref curr, end))
80-
ObjectUtils.ReallocateOutput(ref output, ref isMemory, ref ptr, ref ptrHandle, ref curr, ref end);
81-
return;
82-
}
83-
84-
// From here on we expect only score-element pairs
85-
// Remaining token count should be positive and even
86-
if (currTokenIdx == input.parseState.Count || (input.parseState.Count - currTokenIdx) % 2 != 0)
87-
{
88-
while (!RespWriteUtils.WriteError(CmdStrings.RESP_SYNTAX_ERROR, ref curr, end))
89-
ObjectUtils.ReallocateOutput(ref output, ref isMemory, ref ptr, ref ptrHandle, ref curr, ref end);
90-
return;
91-
}
135+
var parsedOptions = false;
92136

93137
while (currTokenIdx < input.parseState.Count)
94138
{
95-
// Score
96-
if (!input.parseState.TryGetDouble(currTokenIdx++, out var score))
139+
// Try to parse a Score field
140+
if (!input.parseState.TryGetDouble(currTokenIdx, out var score))
97141
{
98-
while (!RespWriteUtils.WriteError(CmdStrings.RESP_ERR_NOT_VALID_FLOAT, ref curr, end))
99-
ObjectUtils.ReallocateOutput(ref output, ref isMemory, ref ptr, ref ptrHandle, ref curr, ref end);
100-
return;
142+
// Try to get and validate options before the Score field, if any
143+
if (!parsedOptions)
144+
{
145+
parsedOptions = true;
146+
if (!GetOptions(ref input, ref currTokenIdx, out options, ref curr, end, ref output, ref isMemory, ref ptr, ref ptrHandle))
147+
return;
148+
continue; // retry after parsing options
149+
}
150+
else
151+
{
152+
// Invalid Score encountered
153+
while (!RespWriteUtils.WriteError(CmdStrings.RESP_ERR_NOT_VALID_FLOAT, ref curr, end))
154+
ObjectUtils.ReallocateOutput(ref output, ref isMemory, ref ptr, ref ptrHandle, ref curr, ref end);
155+
return;
156+
}
101157
}
102158

159+
parsedOptions = true;
160+
currTokenIdx++;
161+
103162
// Member
104163
var memberSpan = input.parseState.GetArgSliceByRef(currTokenIdx++).ReadOnlySpan;
105164
var member = memberSpan.ToArray();

‎libs/server/Resp/KeyAdminCommands.cs

+27-1
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,32 @@ private bool NetworkEXISTS<TGarnetApi>(ref TGarnetApi storageApi)
137137
return true;
138138
}
139139

140+
bool TryGetExpireOption(ReadOnlySpan<byte> item, out ExpireOption option)
141+
{
142+
if (item.EqualsUpperCaseSpanIgnoringCase("NX"u8))
143+
{
144+
option = ExpireOption.NX;
145+
return true;
146+
}
147+
if (item.EqualsUpperCaseSpanIgnoringCase("XX"u8))
148+
{
149+
option = ExpireOption.XX;
150+
return true;
151+
}
152+
if (item.EqualsUpperCaseSpanIgnoringCase("GT"u8))
153+
{
154+
option = ExpireOption.GT;
155+
return true;
156+
}
157+
if (item.EqualsUpperCaseSpanIgnoringCase("LT"u8))
158+
{
159+
option = ExpireOption.LT;
160+
return true;
161+
}
162+
option = ExpireOption.None;
163+
return false;
164+
}
165+
140166
/// <summary>
141167
/// Set a timeout on a key.
142168
/// </summary>
@@ -169,7 +195,7 @@ private bool NetworkEXPIRE<TGarnetApi>(RespCommand command, ref TGarnetApi stora
169195

170196
if (parseState.Count > 2)
171197
{
172-
if (!parseState.TryGetEnum(2, true, out expireOption) || !expireOption.IsValid(ref parseState.GetArgSliceByRef(2)))
198+
if (!TryGetExpireOption(parseState.GetArgSliceByRef(2).ReadOnlySpan, out expireOption))
173199
{
174200
var optionStr = parseState.GetString(2);
175201

0 commit comments

Comments
 (0)