Hello!
On Fri, May 18, 2012 at 4:14 AM, Sparsh Gupta <spars...@gmail.com> wrote:
> Hi Yichun
>
> Thanks again for all the motivation and help all the way. I made my
> first every Lua script and I am loving it already. It seems to be fast
> and working as expected. I am sure there is tons of improvements I can
> do in it and I will learn a ton from you.
>
> I am attaching my lua script with this email which is quite well
> commented. Please feel free if you have any questions
>
> Would love to hear your comments and suggestions to improve this for speed.
>
I reviewed your Lua code briefly on the train yesterday. And here's
the things that I think can be improved in your code:
1. You're using the set_keepalive method of the resty.memcached and
resty.mysql objects in the wrong way. For example, the calling
sequence for the "memc" object in your code looks like this:
local memc = memcached:new()
memc:set_keepalive(0, 100)
memc:connect("127.0.0.1", 11211)
memc:get(key)
This is incorrect because the set_keepalive method does an action
which is putting the current connection into the connection pool. But
you called it before calling the connect method, which means that
you're trying to put a connection that has not yet established into
the pool, that will certainly fail. If you check the return values of
the set_keepalive method, like this,
local ok, err = memc:set_keepalive(0, 100)
if not ok then
ngx.log(ngx.ERR, "failed to set keepalive: ", err)
return
end
then you'll see the following error message in your error.log:
[error] ... failed to set keepalive: closed
That is, the current connection is in the "closed" state. The correct
calling sequence is like
local memc = memcached:new()
memc:connect("127.0.0.1", 11211)
memc:get(key)
memc:set_keepalive(0, 100)
After calling the set_keepalive method, the current memc object will
be inactive and any subsequent attempts of using this object will
result in the "closed" error message unless "connect" is called again.
If you do not call the "set_keepalive" nor "close" methods after
calling "connect", then the "close" method will be called
automatically when the "memc" object is freed by the Lua garbage
collector. So, in your calling sequence, there is no connection pool
love at all.
Check out the documentation of lua-resty-memcached and lua-resty-mysql
for more details:
https://github.com/agentzh/lua-resty-memcached
https://github.com/agentzh/lua-resty-mysql
2. Always check the return values of every method call that may fail,
including the "set_keepalive" method mentioned above, and print the
error message returned from the method to the nginx error.log file.
This will greatly simplify debugging issues when something unexpected
happen. I see you're using the exception handing pattern like
if err then
ngx.print(finish)
return
end
which should really be
if err then
ngx.print(finish)
ngx.log(ngx.ERR, "failed to do something: ", err)
return
end
So that you can see the error messages in error.log, otherwise this
information will get lost and you will never know if anything goes
wrong in production.
3. I see you do a lot of (expensive) string concatenation operations
in your Lua code, using a coding pattern like this:
local final = '...'
for ... do
final = final .. '...'
end
ngx.print(final)
This tends to run really slowly. You can eliminate string
concatenations altogether by means of (array-like) Lua tables, as in
local final_bits = {'...'}
for ... do
table.insert(final_bits, '...')
end
ngx.print(final_bits)
That is, collecting all the string segments into a Lua table, and
finally emit it as a whole by calling the ngx.print method. The
ngx.print method supports (array-like) Lua tables as its arguments and
it will send each string bits in the Lua tables directly to the
underlying socket send buffers without doing string concatenation
operations on the Lua land.
4. I see you roll out your own definition of the urldecode function,
which is unnecessary because ngx_lua already provides the
ngx.unescape_uri method which does the same job and should be more
efficient. See http://wiki.nginx.org/HttpLuaModule#ngx.unescape_uri
for more details.
5. I see you are calling operations like ngx.req.get_headers() again
and again while iterating every row returned from the mysql query but
the return values of ngx.req.get_headers() will not change across the
iterations. You should instead call ngx.req.get_headers() outside of
the loop and save the results into a Lua variable in the outer scope
and reference this variable directly inside the loop.
6. I suggest you put all your own Lua function definitions (like your
"trim" function) into a separate Lua module file. For example, just
like the resty.redis module:
https://github.com/agentzh/lua-resty-redis/blob/master/lib/resty/redis.lua#L1
and then just "require" your Lua module from within the top-level .lua
script file executed by the content_by_lua directive (or its friends).
This approach is going to run faster because Lua modules are only
loaded once upon the first request that requires it.
7. Always specify the "jo" regex options when your regex is constant.
For example,
local url, n = ngx.re.gsub(reqURL,
[[^(https?:\/\/)(?:w{3}\.)?(.*?)(?:\/(?:home|default|index)\..{3,4}|\/$)?(?:\/)?(?:(?:\?\#.*)|(\?.*?)?(?:#.*)?)$]],
"$1$2$3", "i")
the regex argument is a Lua literal string and thus is constant, and
you will get much better performance when adding the "j" and "o" regex
options, as in
local url, n = ngx.re.gsub(reqURL, [[...]], "$1$2$3", "ijo")
On the other hand, if the regex argument has unlimited variants, then
do not specify the "jo" options. For example, for those regexes coming
from your MySQL store, if they have unlimited variants, then do not
try to cache the regex compilation results via the "o" option, because
it will then exhaust the regex cache with a fixed size.
8. It's common practice to save table entry lookups by aliasing the
long names of Lua APIs via local Lua variables. For example, instead
of writing
for ... do
ngx.re.match(...)
end
we usually write
local re_match = ngx.re.match
for ... do
re_match(...)
end
then we can save at least two Lua table lookups for *every* loop
iteration, which can give measurable performance boost.
There may be other nits in your code that I haven't mentioned yet. But
I think this is already a good start anyway and you can optimize
further from here.
BTW, because the suggestions given here may also beneficial to other
Lua programmers, and there's no sensitive information in *this* mail,
I've cc'd the openresty mailing list.
Best regards,
-agentzh