[Compatibility] Added INCRBYFLOAT command

[Vijay-Nirmal]

Adding INCRBYFLOAT command to garnet

• Add INCRBYFLOAT command
• Add Test cases
• Add documentation

Question:

Do you guys have any optimal way to double to ASCII byte?

Below is a benchmark with 2 approaches.

DoubleToSpanWithMathAbsRound => Same code as the PR
DoubleToSpanWithDiyFp => Similar code as string.Format("{0:0.####}"). Below are the links to the code I used from .dotnet runtime repo
Number.DiyFp.cs
Number.Grisu3.cs - TryRunCounted
DoubleToSpanWithStringFormat => Using doubleValue.ToString("0.###############") and using buffer copy to move the value to input span
DoubleToSpanWithUtf8Formatter => Using Utf8Formatter
DoubleToSpanWithTryFormat => Using doubleValue.TryFormat(buffer, out var bytesWritten, "0.#################")

Method	Value	Mean	Error	StdDev	Gen0	Allocated
DoubleToSpanWithMathAbsRound	10.23	32.27 ns	0.399 ns	0.333 ns	-	-
DoubleToSpanWithDiyFp	10.23	51.40 ns	0.619 ns	0.579 ns	-	-
DoubleToSpanWithStringFormat	10.23	121.94 ns	1.474 ns	1.307 ns	0.0017	32 B
DoubleToSpanWithUtf8Formatter	10.23	142.52 ns	2.637 ns	2.590 ns	-	-
DoubleToSpanWithTryFormat	10.23	122.79 ns	2.079 ns	1.843 ns	-	-
DoubleToSpanWithMathAbsRound	12.023642000155785	172.41 ns	3.402 ns	4.303 ns	-	-
DoubleToSpanWithDiyFp	12.023642000155785	51.86 ns	0.928 ns	0.868 ns	-	-
DoubleToSpanWithStringFormat	12.023642000155785	111.17 ns	2.158 ns	2.309 ns	0.0029	56 B
DoubleToSpanWithUtf8Formatter	12.023642000155785	188.86 ns	3.793 ns	3.895 ns	-	-
DoubleToSpanWithTryFormat	12.023642000155785	121.57 ns	1.504 ns	2.825 ns	-	-
DoubleToSpanWithMathAbsRound	85.98498198191919	161.47 ns	1.592 ns	1.330 ns	-	-
DoubleToSpanWithDiyFp	85.98498198191919	50.77 ns	0.834 ns	0.740 ns	-	-
DoubleToSpanWithStringFormat	85.98498198191919	107.39 ns	1.761 ns	1.647 ns	0.0029	56 B
DoubleToSpanWithUtf8Formatter	85.98498198191919	204.33 ns	1.950 ns	1.728 ns	-	-
DoubleToSpanWithTryFormat	85.98498198191919	111.06 ns	1.260 ns	1.052 ns	-	-
DoubleToSpanWithMathAbsRound	152.3645	59.95 ns	0.985 ns	0.922 ns	-	-
DoubleToSpanWithDiyFp	152.3645	58.23 ns	1.142 ns	1.269 ns	-	-
DoubleToSpanWithStringFormat	152.3645	128.90 ns	2.206 ns	1.956 ns	0.0019	40 B
DoubleToSpanWithUtf8Formatter	152.3645	220.70 ns	4.394 ns	5.557 ns	-	-
DoubleToSpanWithTryFormat	152.3645	136.13 ns	2.682 ns	3.193 ns	-	-
DoubleToSpanWithMathAbsRound	456621336.22	82.08 ns	1.669 ns	1.855 ns	-	-
DoubleToSpanWithDiyFp	456621336.22	44.48 ns	0.425 ns	0.355 ns	-	-
DoubleToSpanWithStringFormat	456621336.22	119.48 ns	1.382 ns	1.293 ns	0.0024	48 B
DoubleToSpanWithUtf8Formatter	456621336.22	248.25 ns	4.419 ns	4.134 ns	-	-
DoubleToSpanWithTryFormat	456621336.22	114.32 ns	2.104 ns	1.968 ns	-	-
DoubleToSpanWithMathAbsRound	9.874(...)2E+20 [21]	249.52 ns	4.870 ns	5.980 ns	-	-
DoubleToSpanWithDiyFp	9.874(...)2E+20 [21]	37.10 ns	0.761 ns	0.712 ns	-	-
DoubleToSpanWithStringFormat	9.874(...)2E+20 [21]	126.49 ns	1.706 ns	1.513 ns	0.0033	64 B
DoubleToSpanWithUtf8Formatter	9.874(...)2E+20 [21]	322.90 ns	2.100 ns	1.753 ns	-	-
DoubleToSpanWithTryFormat	9.874(...)2E+20 [21]	138.64 ns	2.705 ns	2.778 ns	-	-

Even though the dotnet runtime approach does better in most cases in the benchmark, I have not done it as part of this PR because of 2 reasons, 1) it has lots of code and 2) for smaller numbers it always performs worse. If you guys like dotnet's approach, I can change it quickly.

Let me know if you guys have any other ideas, I can implement and compare the performance.

[badrishc]

Why not use Utf8Parser.TryParse? The open PR #631 also uses it.
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Float.cs#L63

[badrishc]

Or, double.TryParse(ReadOnlySpan<byte> utf8Text, out double result). This also goes from byte[] to double.

[badrishc]

For other direction, check this:

https://learn.microsoft.com/en-us/dotnet/api/system.buffers.text.utf8formatter.tryformat?view=net-8.0

[badrishc]

might be best to just convert the double to the ROS with a sufficiently large buffer, to know how many bytes it uses. hopefully the conversion will not need to be repeated.

[Vijay-Nirmal]

@badrishc I have added the benchmark results with Utf8Formatter. In my benchmark, it is the slowest option. I've shared below my benchmark method for using Utf8Formatter, please let me know if I made any mistake in my benchmark. Note: We can't use exponential notation.

[Benchmark]
public byte NumOfCharInDoubleWithUtf8Formatter()
{
    Span<byte> buffer = stackalloc byte[327];  // 309 (max digits) + 1 (dot) + 17 (precision)
    Utf8Formatter.TryFormat(Value, buffer, out var bytesWritten, new StandardFormat('f', 17));
    buffer = buffer.Slice(0, bytesWritten).TrimEnd((byte)'0').TrimEnd((byte)'.');
    return buffer[0];
}

[badrishc]

NumOfCharInDoubleWithMathAbsRound seems better as high-perf scenarios might involve smaller doubles. cc @PaulusParssinen for thoughts on this.

[badrishc]

Nicely done PR, approved!

[badrishc]

Please resolve the conflicts and we can move forward.

[badrishc]

@Vijay-Nirmal - there seems to be a perf regression with this PR, for Increment:

BDN paramters: -f BDN.benchmark.Resp.RespParseStress.Increment

main:

Method	Mean	Error	StdDev	Allocated
Increment	34.80 us	0.354 us	0.314 us	-

PR:

Method	Mean	Error	StdDev	Allocated
Increment	44.36 us	0.842 us	0.703 us	-

Analysis

It seems this is related to the stackalloc of the larger sized buffer for doubles in the same method (NetworkIncrement). My suggestion is that we separate out NetworkIncrementByFloat into its own method. Separating it out will also eliminate a couple of condition checks in the critical path of INCR, which is more perf-sensitive than INCRBYFLOAT.

[vazois]

Can you please add a slot verification test in ClusterSlotVeficationTests?

[Vijay-Nirmal]

@badrishc Interesting, For Increment, isFloat will always be false. Then how come the stackalloc is impacting?

@badrishc Is there any thumb rule on when to add RespParseStress?

@vazois That a new things to me, I will add it, also same question Is there any thumb rule on when to add ClusterSlotVeficationTests?

I will do everything this weekend

[badrishc]

@badrishc Interesting, For Increment, isFloat will always be false. Then how come the stackalloc is impacting?

I tried making MaximumFormatDoubleLength equal to MaximumFormatInt64Length and that seems to bring the perf back to closer to original levels, hence the suspicion.

@badrishc Is there any thumb rule on when to add RespParseStress?

Ideally for all "fast" commands we add so that we can track future perf regressions. However it is understandably extra work, so it is mostly on a case-by-case basis for now.

[vazois]

@badrishc Interesting, For Increment, isFloat will always be false. Then how come the stackalloc is impacting?

@badrishc Is there any thumb rule on when to add RespParseStress?

@vazois That a new things to me, I will add it, also same question Is there any thumb rule on when to add ClusterSlotVeficationTests?

I will do everything this weekend

I made some updates and additions in this PR (#709) for slot verification.
If you are adding a multi key command you can check how MSET and MGET is being tested. Single key commands should be easier to understand by check SET and GET redirection tests.
You can also check the description of this PR (#474) if you want to learn more about the redirection logic.

[Vijay-Nirmal]

@badrishc Moved INCRBYFLOAT to a separate NetworkIncrementByFloat

@vazois #474 Has very good info, it will be good if we add the same info in Dev doc for slot migration.

[Vijay-Nirmal]

@vazois Added ClusterSlotVeficationTests. Can you review and approve the PR?

[badrishc]

Can you please add a slot verification test in ClusterSlotVeficationTests?

We will merge this PR now. Vijay - if you can add the ClusterSlotVerificationTest in a separate PR, that would be great.

[Vijay-Nirmal]

@badrishc Already added ClusterSlotVerificationTest in this PR itself

[nightroman]

@Vijay-Nirmal Presumably "-" should have been changed to "+".

Currently INCRBYFLOAT may look like not implemented with the minus sign
https://microsoft.github.io/garnet/docs/commands/api-compatibility

[nightroman]

This PR should fix this, if accepted #757