Skip to content

EVAL addReply() & redisProtocolToLuaType() can be one step #379

Open
JakSprats opened this Issue Mar 10, 2012 · 4 comments

3 participants

@JakSprats

currently lua scripts call a client that writes a response like (if the response were: [3,4,5])
:3\r\n:4\r\n:5\r\n
this string is then parsed in redisProtocolToLuaType() to be 3 integers, which are put onto lua's stack.

The first step could push an integer onto the stack instead of writing ":3\r\n" by overriding the logic in addReply() to have a different behaviour if the redisClient is the server.lua_client. This skips doing a write (":3/r/n") and a parse (":3\r\n")

@JakSprats

bug #392 was a repeat of this one, it has an alternate explanation of what I was trying to say and may be worth looking at.

@pietern
pietern commented Mar 15, 2012

I agree this needs to be tackled, but I want to take a strong look at the existing networking code and see if we can extract a unified API that either writes a reply or puts a reply on the Lua stack. The protocol writing and parsing for every call is pretty low hanging fruit performance-wise ;-)

@JakSprats

Hi Pieter,

good stuff, you understood exactly what I meant :)

good luck on it, it is kind of a lot of code, but once refactored, should be much less code, so that is a good thing too.

as always thanks,

Russ (aka: jaksprats)

@antirez
Owner
antirez commented Oct 15, 2015

Things against this proposal, now very old btw:

  1. Profiling shows parsing the reply is not the critical path performance wise. I was very surprised, but tested it multiple times.
  2. Refactoring such this is going to be huge, and given 1 ...
  3. There is a much more interesting way to speedup things in this regard which is, for common commands, to provide directly a Lua implementation, so that the command is not dispatched at all inside Redis but simply re-implemented. However I tested even this and like in "1" I did not noticed so great speedups.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.