VNSWRR reduces memory usage with GCD

[jizhuozhi]

#1667

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[chobits]

We are best to move the logic of caculating g, ngx_http_upstream_gcd function () and definition of g variable into modules/ngx_http_upstream_vnswrr_module/*.[c or h]. Otherwise you will added too many T_NGX_XXXX macro into nginx core to include your code, which is hard to maintain.

Caculating g may be here as following :

ngx_http_upstream_init_vnswrr
{
...
    if (ngx_http_upstream_init_round_robin(cf, us) != NGX_OK) {
        return NGX_ERROR;
    }

    /* maybe here to insert your logic of caculating g */
}

[jizhuozhi]

Thanks, I will fix that :)

[jizhuozhi]

Hello @chobits , I tried to move this calculation logic, but I found it difficult, because of the same control flow I have to make a new copy (guard statement, filter condition, etc.), and g(gcd) and w( total_weight) is a feature of this list, so I think it is appropriate to calculate it together with w

[chobits]

gcd can be calculted firstly as a local variable in ngx_http_upstream_init_vnswrr.
Then after vpeers ( and backup vpeers) have been created, gcd varirable can be saved into uvnscf

[jizhuozhi]

gcd can be calculted firstly as a local variable in ngx_http_upstream_init_vnswrr. Then after vpeers ( and backup vpeers) have been created, gcd varirable can be saved into uvnscf

Hello @chobits , this has been fixed. But when I try to test my submit, I get this error

tests/nginx-tests/tengine-tests/ngx_http_upstream_vnswrr.t .... 
Dubious, test returned 2 (wstat 512, 0x200)

I'm new to perl, so I don't know how to fix it, could you help me?

[chobits]

added TEST_NGINX_VERBOSE=yes before command prove, it will output debug log of Test::Nginx.pm.
also enable -v option of prove

as following:

# tengine_source_dir/tests/nginx-tests/nginx-tests/lib
nginx_tests_lib_path=$(pwd)/tests/nginx-tests/nginx-tests/lib
nginx_binary=$(pwd)/output/sbin/nginx

proxy_connect_test_cases=tests/nginx-tests/nginx-tests/*resolver*

TEST_NGINX_VERBOSE=yes \
TEST_NGINX_UNSAFE=yes \
TEST_NGINX_BINARY=$nginx_binary \
prove -v -I $nginx_tests_lib_path $proxy_connect_test_cases

[jizhuozhi]

Hello, @chobits . I carefully read the README, and repeatedly confirmed to configure, compile and install according to the installation chapters. But when I use gdb to debug, it always prompts me that the corresponding functions and symbols cannot be found. Later, I checked the generated object and found that the vnswrr module was not compiled, it just compiled the nginx native module.

When I searched for vnswrr globally in the project, I found that I needed to add --add-module=./modules/ngx_http_upstream_vnswrr_module to install the corresponding module, and found my compilation error according to it, and it works fine after repairing Compile passed.

I think there should be a corresponding prompt or accessible Contribute documentation in the README to tell newbies that they need to add configuration options to compile their own modified modules (and the default ./configure may not contain Tengine modules)

Now my modification can be tested normally, thanks :)

[chobits]

with updated my changes. cases passed

 $ git diff
diff --git a/modules/ngx_http_upstream_vnswrr_module/ngx_http_upstream_vnswrr_module.c b/modules/ngx_http_upstream_vnswrr_module/ngx_http_upstream_vnswrr_module.c
index b7689f8..20fe0a7 100644
--- a/modules/ngx_http_upstream_vnswrr_module/ngx_http_upstream_vnswrr_module.c
+++ b/modules/ngx_http_upstream_vnswrr_module/ngx_http_upstream_vnswrr_module.c
@@ -235,6 +235,8 @@ ngx_http_upstream_init_vnswrr(ngx_conf_t *cf,
         ubvnscf->last_number = NGX_CONF_UNSET_UINT;
         ubvnscf->last_peer = NULL;

+    if (us->servers) {
+        server = us->servers->elts;
         g = 0;
         for (i = 0; i < us->servers->nelts; i++) {
             if (!server[i].backup) {
@@ -250,6 +252,7 @@ ngx_http_upstream_init_vnswrr(ngx_conf_t *cf,

         ubvnscf->gcd = g;

+    }
         uvnscf->next = ubvnscf;
         uvnscf->next = ubvnscf;

         if (!backup->weighted) {
diff --git a/tests/nginx-tests/tengine-tests/ngx_http_upstream_vnswrr.t b/tests/nginx-tests/tengine-tests/ngx_http_upstream_vnswrr.t
index a29454e..6228ec0 100644
--- a/tests/nginx-tests/tengine-tests/ngx_http_upstream_vnswrr.t
+++ b/tests/nginx-tests/tengine-tests/ngx_http_upstream_vnswrr.t
@@ -116,7 +116,7 @@ http {

 EOF

-$t->try_run('no upstream vnswrr')->plan(10);
+$t->try_run('no upstream vnswrr')->plan(13);

 $ sh runtest.sh 2>/dev/null
./tests/nginx-tests/tengine-tests/ngx_http_upstream_vnswrr.t .. ok
tests/nginx-tests/tengine-tests/vnswrr4dynamic_ups.t .......... ok
All tests successful.
Files=2, Tests=23, 15 wallclock secs ( 0.02 usr  0.00 sys +  0.17 cusr  0.01 csys =  0.20 CPU)
Result: PASS

[jizhuozhi]

Hello, @wangfakang. I refactored my code to make calculating gcd and backup gcd in single loop, PLAT.

[chobits]

If no other changes, you need to squash all commits into one. Then merged commit log will be clean.

(may git rebase -i .. && git push --force )

[jizhuozhi]

Hello, @chobits. All commits in this PR have already been squashed, PLAT

[chobits]

merged. ty for ur contribution！