perf(router): increase lua_regex_cache_max_entries and show warning if too small
#9624
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mayocream
changed the title
lua_regex_cache_max_entries and show warning if too smalllua_regex_cache_max_entries and show warning if too small
mayocream
changed the title
lua_regex_cache_max_entries and show warning if too smalllua_regex_cache_max_entries and show warning if too small
chronolaw
reviewed
Oct 27, 2022
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
mayocream
commented
Oct 28, 2022
mayocream
force-pushed
the
fix/lua_regex_cache_max_entries
branch
3 times, most recently
from
324d3e9
to
3734160
Compare
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
chronolaw
approved these changes
Oct 28, 2022
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
bungle
reviewed
Oct 28, 2022
kong/router/traditional.lua
Outdated
| @@ -1448,6 +1450,20 @@ function _M.new(routes, cache, cache_neg) | |||
| local match_sources = not isempty(plain_indexes.sources) | |||
| local match_destinations = not isempty(plain_indexes.destinations) | |||
|
|
|||
| -- warning about the regex cache size being too small | |||
| if not lua_regex_cache_max_entries then | |||
| lua_regex_cache_max_entries = kong.configuration.nginx_http_lua_regex_cache_max_entries or 1024 | |||
There was a problem hiding this comment.
Should we default here to 8192 too?
There was a problem hiding this comment.
This seems non-sense because OpenResty's default value of lua_regex_cache_max_entries is 1024 (https://github.com/openresty/lua-nginx-module/blob/master/README.markdown#lua_regex_cache_max_entries), we set the nginx_http_lua_regex_cache_max_entries to 8192, but the user could overwrite it.
mayocream
force-pushed
the
fix/lua_regex_cache_max_entries
branch
from
ae2eb90
to
4e29ac0
Compare
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
dndx
reviewed
Oct 31, 2022
kong/router/traditional.lua
Outdated
| and tonumber(kong.configuration.nginx_http_lua_regex_cache_max_entries) or 1024 | ||
| end | ||
|
|
||
| if worker_id() == 0 and regex_uris[0] > lua_regex_cache_max_entries then |
There was a problem hiding this comment.
I think we should also be checking for regex_uris[0] * 2 > lua_regex_cache_max_entries here, if user sets lua_regex_cache_max_entries to be exactly regex_uris[0] we can still expect issues regarding regex cache trashing.
mayocream
force-pushed
the
fix/lua_regex_cache_max_entries
branch
from
58ccaa3
to
546e4fd
Compare
chronolaw
reviewed
Oct 31, 2022
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
dndx
approved these changes
Nov 1, 2022
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple)
Click to expandDownload Artifacts for detailed results and interactive SVG flamegraphs. |
|
Hi @mayocream , |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When a user sets too many regex routes and
lua_regex_cache_max_entriesis set too low (or kept at default 1024), a performance issue might occur due to regex trashing.OpenResty claims that it will emit a warning when the limit has been exceeded, but it doesn't.
Fix FTI-4455