Add jit_stack_size option #44
Conversation
lib/resty/core/regex.lua
Outdated
| @@ -162,6 +165,11 @@ local function get_max_regex_cache_size() | |||
| return max_regex_cache_size | |||
| end | |||
|
|
|||
| function _M.opt(option, value) | |||
| if option == "jit_stack_size" then | |||
| C.ngx_http_lua_ffi_set_jit_stack_size(value) | |||
There was a problem hiding this comment.
Style: we use 4-space indentation in this project.
|
The function body now uses 4 spaces instead of 2 |
|
Regarding tests for resty-core, can we reuse the tests from lua-nginx-module's repo somehow or do you want me to copy them over to this PR? |
|
@alubbe Yes, we can run ngx_lua's test suite using lua-resty-core by specifying the following environment beforehand: export TEST_NGINX_INIT_BY_LUA="package.path = '$PWD/../lua-resty-core/lib/?.lua;$PWD/../lua-resty-lrucache/lib/?.lua;;' .. (package.path or '') require 'resty.core'"assuming that lua-resty-core and lua-resty-lrucache are cloned in the same (parent) directory as ngx_lua. |
|
Awesome, they pass locally. |
|
@alubbe I think we still need some test cases to ensure that this new Lua API is indeed JIT compiled, as with the existing tests of this project. |
lib/resty/core/regex.lua
Outdated
| @@ -135,6 +135,9 @@ ffi.cdef[[ | |||
| unsigned char *dst); | |||
|
|
|||
| uint32_t ngx_http_lua_ffi_max_regex_cache_size(void); | |||
|
|
|||
| void ngx_http_lua_ffi_set_jit_stack_size(int size); | |||
|
|
|||
There was a problem hiding this comment.
Shall we remove this empty line?
|
I've updated this PR to include the exact same functionality and error messages as the other PR. |
|
@alubbe I think we still need a simple test to ensure LuaJIT's JIT compiler does not complain about any NYI when running this API function in a loop (see other tests for such examples). Thanks! |
|
I've been thinking about this for a while, and, if possible, I'd simply like to re-run all find, match and sub tests with this option enabled. It would take care of NYI checks and other potential issues that we might have missed. Is there a simple way of doing this or do we need to duplicate every single test? If the latter, maybe we should add only one test per test file. |
|
@alubbe I think you can just write typical tests. I don't mind duplications in the test suite since tricky tests often hide bugs of their own :) Simplicity takes preference in testing. |
|
I have added two NYI tests. Presumely, they can't pass until openresty/lua-nginx-module#782 is merged, right? |
alubbe
force-pushed
the
jit_stack_size
branch
2 times, most recently
from
55e1a7b to
59bcec0
Compare
|
I've updated the travis file to use my other branch. The PCRE allocation fails, even though it works locally on my machine (I ran a minimal example, not the test suite). Could you help me figure out what's going on? Also, my perl-foo was not strong enough to add a switch to use ngx.re.opt only for the last two tests. Or do you want to create a whole new file for these tests? Beyond those two points, this PR should be finished - it's been working fine in production for several weeks now. |
|
ping @agentzh |
t/re-find.t
Outdated
| --- config | ||
| location = /re { | ||
| access_log off; | ||
| content_by_lua ' |
There was a problem hiding this comment.
New tests should always use the *_by_lua_block {} form :)
|
@alubbe I see the tests in Travis CI are failing. Maybe you should rebase your lua-resty-core and lua-nginx-module branches to the their latest masters? |
alubbe
force-pushed
the
jit_stack_size
branch
from
59bcec0 to
d13e3d8
Compare
|
I have just rebased, but the issues are still the same as described in #44 (comment) and openresty/lua-nginx-module#782 (comment) |
alubbe
force-pushed
the
jit_stack_size
branch
from
fdca75a to
bda2435
Compare
|
@alubbe What I'm seeing in your lua-resty-core PR's travis CI report is actually a compilation error: https://travis-ci.org/openresty/lua-resty-core/jobs/175504983 Please provide links to the travis CI report you're talking about. I'm not sure what you mean here. |
Maybe you'll find the |
resty.core uses resty.lrucache instead of the C level cache
Cleans up the total diff of this PR
Thanks to sleep deprivation
|
It broke because of the new commit removing the empty lines from the test files - rebased! |
|
@alubbe Merged with the following extra patch. Thanks! diff --git a/.travis.yml b/.travis.yml
index 5c70fc8..f99be4c 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -56,7 +56,7 @@ install:
- git clone https://github.com/openresty/openresty.git ../openresty
- git clone https://github.com/openresty/nginx-devel-utils.git
- git clone https://github.com/simpl/ngx_devel_kit.git ../ndk-nginx-module
- - git clone -b pcre_jit_stack_size https://github.com/alubbe/lua-nginx-module.git ../lua-nginx-module
+ - git clone https://github.com/openresty/lua-nginx-module.git ../lua-nginx-module
- git clone https://github.com/openresty/no-pool-nginx.git ../no-pool-nginx
- git clone https://github.com/openresty/echo-nginx-module.git ../echo-nginx-module
- git clone https://github.com/openresty/lua-resty-lrucache.git
diff --git a/lib/ngx/re.lua b/lib/ngx/re.lua
index 93e29c9..28e9ed0 100644
--- a/lib/ngx/re.lua
+++ b/lib/ngx/re.lua
@@ -36,8 +36,8 @@ local split_ctx = new_tab(0, 1)
ffi.cdef[[
- int ngx_http_lua_ffi_set_jit_stack_size(int size, unsigned char *errstr,
- size_t *errstr_size);
+int ngx_http_lua_ffi_set_jit_stack_size(int size, unsigned char *errstr,
+ size_t *errstr_size);
]]
@@ -235,8 +235,8 @@ end
function _M.opt(option, value)
if option == "jit_stack_size" then
if not is_regex_cache_empty() then
- return error("changing jit stack size is not allowed when some " ..
- "regexs have already been compiled and cached")
+ error("changing jit stack size is not allowed when some " ..
+ "regexs have already been compiled and cached")
end
local errbuf = get_string_buf(MAX_ERR_MSG_LEN)
@@ -247,13 +247,12 @@ function _M.opt(option, value)
if rc == FFI_OK then
return
-
- else
- return error(ffi_str(errbuf, sizep[0]))
end
+
+ error(ffi_str(errbuf, sizep[0]))
end
- return error("unrecognized option name")
+ error("unrecognized option name")
end
diff --git a/lib/ngx/re.md b/lib/ngx/re.md
index 153daba..c1df8ed 100644
--- a/lib/ngx/re.md
+++ b/lib/ngx/re.md
@@ -176,7 +176,7 @@ The `jit_stack_size` cannot be set to a value lower than PCRE's default of 32K.
This method requires the PCRE library enabled in Nginx.
-This feature was first introduced in the `XXX` release.
+This feature was first introduced in the `v0.1.12` release.
[Back to TOC](#table-of-contents)
diff --git a/t/re-find.t b/t/re-find.t
index b896949..772ec3b 100644
--- a/t/re-find.t
+++ b/t/re-find.t
@@ -260,4 +260,3 @@ matched: 234
NYI
--- error_log eval
qr/\[TRACE \d+ content_by_lua\(nginx\.conf:\d+\):4 loop\]/
-
diff --git a/t/re-opt.t b/t/re-opt.t
index d2bda46..18f66a3 100644
--- a/t/re-opt.t
+++ b/t/re-opt.t
@@ -69,12 +69,13 @@ __DATA__
error: pcre_exec() failed: -27
--- no_error_log
[error]
+--- timeout: 10
=== TEST 2: increase jit_stack_size
--- http_config eval
-<<_EOC_;
+qq{
lua_package_path "$::pwd/lib/?.lua;../lua-resty-lrucache/lib/?.lua;;";
init_by_lua_block {
-- local verbose = true
@@ -97,7 +98,7 @@ error: pcre_exec() failed: -27
local ngx_re = require "ngx.re"
ngx_re.opt("jit_stack_size", 128 * 1024)
}
-_EOC_
+}
--- config
location /re {
content_by_lua_block {
@@ -124,6 +125,7 @@ from: 1
to: 1563
--- no_error_log
[error]
+--- timeout: 10
@@ -153,6 +155,7 @@ qr/changing jit stack size is not allowed when some regexs have already been com
--- grep_error_log_out eval
["", "changing jit stack size is not allowed when some regexs have already been compiled and cached\n"]
+--- timeout: 10
@@ -169,7 +172,8 @@ qr/changing jit stack size is not allowed when some regexs have already been com
}
--- request
GET /re
---- response_body
-unrecognized option name
+--- response_body_like chomp
+unrecognized option name$
--- no_error_log
[error]
+--- timeout: 10
|
|
Perfect, it's been a long journey :D |
Complements openresty/lua-nginx-module#782