Skip to content

Conversation

@spacewander
Copy link
Member

No description provided.

local function do_cmd(self, cmd, ...)
local module_prefix = rawget(self, "_module_prefix")
if module_prefix then
self._module_prefix = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad indent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doujiang24
Fixed.


function _M.register_module_prefix(mod)
_M[mod] = function(self)
self._module_prefix = mod .. "."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self._module_prefix = mod .. "."
self._module_prefix = mod

move the .. "." to line 414: https://github.com/openresty/lua-resty-redis/pull/211/files#diff-adb515948c05651051c83fd6fb3ce7ceR414
to avoid creating a new string for better performance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doujiang24
There will be more new strings, as each time the cmd argument need to be a concatenated string in line 414.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacewander I don't think so.
it will generate a new string when calling the module function, like redis:bp().
and generate another string when calling the method, like :add().

that means will generate two string in redis:bp:add().

while module_prefix .. "." .. cmd will generate one string.

Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this way may have better performance, thoughts?

local dot_cmds = {
    "bf.add", "bf.exists",
}

for i = 1, #dot_cmds do
    local cmd = dot_cmds[i]
    local name = string.gsub(cmd, ".", "_")

    _M[name] =
        function (self, ...)
            return _do_cmd(self, cmd, ...)
        end
end


function _M.register_module_prefix(mod)
_M[mod] = function(self)
self._module_prefix = mod .. "."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacewander I don't think so.
it will generate a new string when calling the module function, like redis:bp().
and generate another string when calling the method, like :add().

that means will generate two string in redis:bp:add().

while module_prefix .. "." .. cmd will generate one string.

@spacewander
Copy link
Member Author

I think this way may have better performance, thoughts?

local dot_cmds = {
    "bf.add", "bf.exists",
}

for i = 1, #dot_cmds do
    local cmd = dot_cmds[i]
    local name = string.gsub(cmd, ".", "_")

    _M[name] =
        function (self, ...)
            return _do_cmd(self, cmd, ...)
        end
end

OK, maybe we can just a much simpler solution like https://github.com/openresty/lua-resty-redis/pull/167/files

@doujiang24
Copy link
Member

OK, maybe we can just a much simpler solution like https://github.com/openresty/lua-resty-redis/pull/167/files

it has two issues:

  1. need a string.gsub call per redis method call.
  2. some redis command may contains _, like SORT_RO: https://redis.io/commands/sort_ro

Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new module function call style: redis:bf():add().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants