openresty / lua-nginx-module

Embed the Power of Lua into NGINX HTTP servers
https://openresty.org/
11.33k stars 2.03k forks source link

optimize: use `ngx_hash_t` to optimize the built-in header look-up process #2246

Closed lynch1981 closed 8 months ago

lynch1981 commented 1 year ago

I hereby granted the copyright of the changes in this pull request to the authors of this lua-nginx-module project.

Get a TODO issue done use ngx_hash_t to optimize the built-in header look-up process for ngx.req.set_header, and etc.

lynch1981 commented 1 year ago

Can you review this PR when you have time:) @zhuizhuhaomeng

zhuizhuhaomeng commented 11 months ago

@lynch1981 This is a good PR for me. Do you have any benchmark data to show the effect of the optimization?

lynch1981 commented 11 months ago

@zhuizhuhaomeng I just started one nginx worker, the benchmark data shows the difference when I call ngx.req.set_header a hundred times (using my old MacBook pro)

lynch@ngx-dev:~/git/t$ ab -c 100 -n 10000 http://127.0.0.1:8888/test

location /test {
    access_by_lua_block {
        for i=1, 100 do
            ngx.req.set_header("Custom-Content-Type" .. i, "text/css")
        end
    }

    echo "ok";
}

before this optimize

Concurrency Level: 100 Time taken for tests: 12.365 seconds Complete requests: 10000 Failed requests: 0 Total transferred: 1260000 bytes HTML transferred: 30000 bytes Requests per second: 808.75 [#/sec] (mean) Time per request: 123.648 [ms] (mean) Time per request: 1.236 [ms] (mean, across all concurrent requests) Transfer rate: 99.51 [Kbytes/sec] received

after this optimize

Concurrency Level: 100 Time taken for tests: 11.464 seconds Complete requests: 10000 Failed requests: 0 Total transferred: 1260000 bytes HTML transferred: 30000 bytes Requests per second: 872.33 [#/sec] (mean) Time per request: 114.636 [ms] (mean) Time per request: 1.146 [ms] (mean, across all concurrent requests) Transfer rate: 107.34 [Kbytes/sec] received

zhuizhuhaomeng commented 10 months ago

@lynch1981 Would you please add more benchmark tests about the built-in header? You can get a real HTTP request from chrome, and then modify some of the headers? We will add the bench data to the git commit log.

zhuizhuhaomeng commented 10 months ago

@lynch1981 would you please add more benchmark tests about the builtin header

lynch1981 commented 10 months ago

@zhuizhuhaomeng I tested it again, and this time there was basically no difference in performance between the two versions. Maybe I need to use openresty xray to analyze it :)

lynch1981 commented 10 months ago

FlameGraph shows that most of the CPU consumption is in ngx_hash_key_lc(), ngx_http_lua_copy_escaped_header() and ngx_http_set_header(), while the CPU consumption of our optimized code only accounts for a small part.

So, sorry, this optimization is not necessary

FlameGraph

perf record -F 99 -p 1039419 -g -- sleep 30
perf script | ./stackcollapse-perf.pl > out.perf-folded
./flamegraph.pl out.perf-folded > perf.svg