perf(router) efficient host header reading #2271
Conversation
We now avoid reading all request headers via `ngx.req.get_headers()` and use the `ngx.var` API instead. In the future, we'll distinguish Host fetching and full headers fetching when arbitrary headers matching will be added to the router.
| @@ -446,25 +446,24 @@ function _M.new(apis) | |||
| end) | |||
|
|
|||
|
|
|||
| local grab_headers = #wildcard_hosts > 0 or next(indexes.plain_hosts) ~= nil | |||
| local grab_host = #wildcard_hosts > 0 or next(indexes.plain_hosts) ~= nil | |||
There was a problem hiding this comment.
might we consider removing this variable altogether and moving this expression to line 582? reducing the number of live local variables is a win (this assumes the commented out print statement on 579 is removed or adjusted as well)
There was a problem hiding this comment.
This is such a hot code path that the intent here has always been to cache whether retrieving any header is necessary at all. Moving this down to 582 prevents this code to JIT (next) and evaluates the same statement over and over again.
There was a problem hiding this comment.
Ah, my fault for misreading the scope of the var. For some reason I thought it was being declared in exec, not new. Ignore me!
| if type(method) ~= "string" then | ||
| return error("arg #1 method must be a string") | ||
| end | ||
| if type(uri) ~= "string" then | ||
| return error("arg #2 uri must be a string") | ||
| end | ||
| if type(headers) ~= "table" then | ||
| return error("arg #3 headers must be a table") | ||
| if host and type(host) ~= "string" then |
There was a problem hiding this comment.
is the host and necessary?
There was a problem hiding this comment.
In the tests, there are occasions where the 3rd argument is ignored for readability.
There was a problem hiding this comment.
type(host) would be nil then, no? So the conditional is not necessary then. But no big deal, you know the codebase better than I. Just trying to grasp everything :)
There was a problem hiding this comment.
You're right, we can probably remove it from here :)
There was a problem hiding this comment.
Actually on second thought, it is better as it is :) The ~= "string" is key here.
We now avoid reading all request headers via
ngx.req.get_headers()anduse the
ngx.varAPI instead. In the future, we'll distinguish Hostfetching and full headers fetching when arbitrary headers matching will
be added to the router.