Performance improvements, better API, minimise allocations

[alicebob]

What's the status of this? Is this stable enough that I can start a branch in miniredis to use the new API?

[dim]

Hey, yes, please go ahead and use the new API, from the feature/v2 branch, it's virtually stable.

I'm currently working on a larger project as a proof of concept and have been updating the API to support all the requirements. There haven't been any breaking changes recently, so I think you are safe to use it.

Please also let me know if you have any comments, I would appreciate your feedback.

On 15 Apr 2017 4:27 pm, "Harmen" notifications@github.com wrote:

What's the status of this? Is this stable enough that I can start a branch in miniredis to use the new API?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bsm/redeo/pull/13#issuecomment-294296713, or mute the thread https://github.com/notifications/unsubscribe-auth/AABA5v_meLy8oTVYD3LZz9U84nnFT-0wks5rwNPPgaJpZM4MXHZE .

[alicebob]

Looking forward to add the streaming stuff to miniredis. But that comes later.

Both src.Client() and client.Ctx got removed. I use that to keep track of authenticated and transaction status. I can use the new context to store the variables, but I don't see how I can get hold of the Client object, given only a *resp.Command (using the /resp library). Should I not use the resp library and do the basic server structure myself (net.Listen and stuff)?

[dim]

@alicebob abow about https://github.com/bsm/redeo/commit/9373c74fb78d3ce4b033d95d509f9a647a991d04 ?

[alicebob]

Thanks, I've updated the code. It compiles, I'm now working through the failing tests. This used to work for the ECHO command:

    msg := r.Arg(0).String()
    out.AppendInlineString(msg)

with the string "hello\nworld". Now that gives this error:

cmd_connection_test.go:41: unexpected error: redigo: bad response line terminator (possible server error or unsupported concurrent read by application)

Changing that to AppendBulkString(msg) solves the test, but shouldn't it work with inlinestring?

Edit: never mind, it always did a out.WriteString(msg), which is the bulk string.

[dim]

Yeah, inline strings must not contain new lines, https://redis.io/topics/protocol

[alicebob]

@dim I got it working in the branch referenced above. Thanks for the help!

I did not look at the internal details of the change, but I noticed a few things during the rework:

• AppendInt() expects an int64. Fine, but that's a bit annoying in some places, such as AppendInt(int64(len(something)))
• I see where you're going with the c.Args() (and c.ArgN() &c.) change, but it never makes things really easier for me. But maybe that's only because the code is build around a simple string slice.
• c.Arg(1).Int() returns an int64, which I don't expect. See above, I guess.
• I can't keep the command (and c.Args()) around in a closure. I suspect the command is reused somewhere to save allocations. In miniredis the command might end up in a closure when executing redis transactions. The command has returned, but the arguments are kept around to be used later when the transaction is executed. Since I basically always copy c.Args() to a string slice anyway it doesn't really matter, but it broke in a few places.

Edit: since for miniredis performance is no issue at all I think the focus of redeo and miniredis are not really the same, and I went for a tiny server included in the repo.

[dim]

Hey again, thanks for the detailed feedback. I would like to explain some of the design choices but please feel free to comment again if you feel strongly about them:

1. The reason behind using int64 values is based on the way Redis in handling numbers internally. On a 64-bit machine, for example, a Redis server might want to return 8796093022208 to the client but calling c.Arg(1).Int() on a 32-bit machine would cause an overflow and result in a wrong value. The only way around this limitation is to use int64s consistently.
2. I agree on c.Args and c.ArgN but I have not found a better solution for this - suggestions are welcome.
3. Yes, command is not re-usable and only exists for the duration of the handler. That's an intentional limitation and it's really pretty important. Redeo was made to power an ultra-low-latency system, avoiding garbage collection is almost essential under these circumstances. There is nothing wrong with copying arguments, if you want to retain a copy of command arguments.

[alicebob]

Those reasons make sense, no surprise. As said, I switched miniredis over to a very basic redis server implementation, because the features which make sense for your use case only distract there. Thanks!