RespServerSession RESP3 update

[prvyk]

This PR has two main goals:

1. Further update RESP3 support.
2. Save on boilerplate in RespServerSession output.

The former is done by fixing various cases for RESP3 null/double and adding verbatim string support. The latter is done by adding output functions to RespServerSession (in a separate file), but the PR holds off on use till performance implications are fully checked. Also some misc. RESP2 updates (redis-cli expects an extra newlines in CLIENT INFO/LIST, these commands should emit Bulk, not simple strings).

[badrishc]

This is a lot of refactoring and depending on .NET's force inlining capability. We need to make sure there are no performance regressions due to e.g. saturating the inline limits that .NET places internally.

Also, see the BDN results here and identify any regressions: ee395dc#comments

There seems to be general degradations all over the place.

[badrishc]

Would it be possible to break this PR up into two parts: (1) the force-inlined-method version of while() ... SendAndReset(); and (2) all the other improvements/bugfixes that have no perf impact. The latter is easier to merge, whereas the former still needs better understanding of perf implications.

[prvyk]

Yes, I was already working on that, will update the PR a bit later.

[TalZaccai]

Great work here! Thanks for spending time on this, looks good to me overall.

A main concern I have is that this is not really tested. We might want to start by adding a RESP3 test case wherever we call ConnectionMultiplexer.Connect(TestUtils.GetConfig()) and set the protocol parameter in GetConfig. Thoughts?

[prvyk]

Great work here! Thanks for spending time on this, looks good to me overall.

A main concern I have is that this is not really tested. We might want to start by adding a RESP3 test case wherever we call ConnectionMultiplexer.Connect(TestUtils.GetConfig()) and set the protocol parameter in GetConfig. Thoughts?

The question is what we test. Do we test that RESP3 output is working at all? Take a command with slightly differing output and ensure it's different? Or maybe test StackExchange is working in RESP3 mode?

There's a test for the first. Maybe we can join in testing the latter two by adding a parameter to certain testcases to set protocol version, and let StackExchange do the output test.

[TalZaccai]

Great work here! Thanks for spending time on this, looks good to me overall.
A main concern I have is that this is not really tested. We might want to start by adding a RESP3 test case wherever we call ConnectionMultiplexer.Connect(TestUtils.GetConfig()) and set the protocol parameter in GetConfig. Thoughts?

The question is what we test. Do we test that RESP3 output is working at all? Take a command with slightly differing output and ensure it's different? Or maybe test StackExchange is working in RESP3 mode?

There's a test for the first. Maybe we can join in testing the latter two by adding a parameter to certain testcases to set protocol version, and let StackExchange do the output test.

Yup I was more concerned with the latter for now, check that the client that we use does not break.

[prvyk]

Great work here! Thanks for spending time on this, looks good to me overall.
A main concern I have is that this is not really tested. We might want to start by adding a RESP3 test case wherever we call ConnectionMultiplexer.Connect(TestUtils.GetConfig()) and set the protocol parameter in GetConfig. Thoughts?

The question is what we test. Do we test that RESP3 output is working at all? Take a command with slightly differing output and ensure it's different? Or maybe test StackExchange is working in RESP3 mode?
There's a test for the first. Maybe we can join in testing the latter two by adding a parameter to certain testcases to set protocol version, and let StackExchange do the output test.

Yup I was more concerned with the latter for now, check that the client that we use does not break.

That turned out to be very useful. There were APIs which ran commands and read their output into variables. These commands could both output directly (so RESP3 change) and could be read, and the APIs was not able to read RESP3 output. The previous RESP3 PR had not accounted for this hack.

This was fixed with a mix of forcing some commands to RESP2 when we know the output is just read into variables (ZMPOP, ZRANGESTORE), and extending some reading code to handle RESP3 types and array-in-array (ProcessRespArrayOutput in Common.cs).

In 515c358 the entire testsuit was temporarily modified to run StackExchange via RESP3 and it passed (the two failures are sporadic and unrelated). This was later reverted to the default RESP2.

ZDIFF has a minor issue where ZDIFF z nx (z exists, nx does not) doesn't output z, but fixing it interfered with some tests so another time.

[TalZaccai]

LGTM! Thanks for addressing all my comments!

[prvyk]

LGTM! Thanks for addressing all my comments!

Two minor changes since then (following 06f6cd0: #1241 showed that some APIs can/should read NULL and the underlying TryReadSignedLengthHeader wasn't updated for RESP3 - added some code for it, it doesn't conflict with that PR, nor with what the function already did (it already accounted for RESP2 NULL, and callers already checked length >= 0 when needed).

It also turned out that HELLO returned proto field as BulkString and not integer, and some client libraries don't like it (they check it to see HELLO 3 worked, and then they get "3", do "3" != 3 and panic). Fixed that. Also, id field was oddly hardcoded when it should be connection id, fixed that too.

[TalZaccai]

Added a few more comments following the latest changes...

[TalZaccai]

Added a few additional comments to the recent changes.

[TalZaccai]

Looks good! Thanks!