Modules: removed extra VM creation per server.

Previously, when js_import was declared in http or stream blocks, an extra copy of the VM instance was created for each server block. This was not needed and consumed a lot of memory for configurations with many server blocks. This issue was introduced in 9b67441 (0.8.6) and was partially fixed for location blocks only in 685b64f (0.8.7).
nginx · Jan 4, 2025 · 61feaa6 · 61feaa6
1 parent 1f8f999
commit 61feaa6
Show file tree

Hide file tree

Showing 5 changed files with 194 additions and 2 deletions.
diff --git a/nginx/ngx_js.c b/nginx/ngx_js.c
@@ -3343,6 +3343,16 @@ ngx_js_merge_vm(ngx_conf_t *cf, ngx_js_loc_conf_t *conf,
     ngx_array_t          *imports, *preload_objects, *paths;
     ngx_js_named_path_t  *import, *pi, *pij, *preload;
 
+    if (prev->imports != NGX_CONF_UNSET_PTR && prev->engine == NULL) {
+        /*
+         * special handling to preserve conf->engine
+         * in the "http" or "stream" section to inherit it to all servers
+         */
+        if (init_vm(cf, (ngx_js_loc_conf_t *) prev) != NGX_OK) {
+            return NGX_ERROR;
+        }
+    }
+
     if (conf->imports == NGX_CONF_UNSET_PTR
         && conf->type == prev->type
         && conf->paths == NGX_CONF_UNSET_PTR
@@ -3851,6 +3861,9 @@ ngx_js_init_conf_vm(ngx_conf_t *cf, ngx_js_loc_conf_t *conf,
         return NGX_ERROR;
     }
 
+    ngx_log_error(NGX_LOG_NOTICE, cf->log, 0, "js vm init %s: %p",
+                  conf->engine->name, conf->engine);
+
     cln = ngx_pool_cleanup_add(cf->pool, 0);
     if (cln == NULL) {
         return NGX_ERROR;
@@ -4039,6 +4052,10 @@ ngx_js_merge_conf(ngx_conf_t *cf, void *parent, void *child,
     ngx_js_loc_conf_t *conf = child;
 
     ngx_conf_merge_uint_value(conf->type, prev->type, NGX_ENGINE_NJS);
+    if (prev->type == NGX_CONF_UNSET_UINT) {
+        prev->type = NGX_ENGINE_NJS;
+    }
+
     ngx_conf_merge_msec_value(conf->timeout, prev->timeout, 60000);
     ngx_conf_merge_size_value(conf->reuse, prev->reuse, 128);
     ngx_conf_merge_size_value(conf->buffer_size, prev->buffer_size, 16384);

diff --git a/nginx/t/js_import2.t b/nginx/t/js_import2.t
@@ -41,6 +41,7 @@ http {
 
         js_set $test foo.bar.p;
 
+        # context 1
         js_import foo from main.js;
 
         location /njs {
@@ -52,11 +53,13 @@ http {
         }
 
         location /test_lib {
+            # context 2
             js_import lib.js;
             js_content lib.test;
         }
 
         location /test_fun {
+            # context 3
             js_import fun.js;
             js_content fun;
         }
@@ -75,6 +78,7 @@ http {
         server_name  localhost;
 
         location /test_fun {
+            # context 4
             js_import fun.js;
             js_content fun;
         }
@@ -114,7 +118,7 @@ $t->write_file('main.js', <<EOF);
 
 EOF
 
-$t->try_run('no njs available')->plan(5);
+$t->try_run('no njs available')->plan(6);
 
 ###############################################################################
 
@@ -124,4 +128,10 @@ like(http_get('/test_fun'), qr/FUN-TEST/s, 'fun');
 like(http_get('/proxy/test_fun'), qr/FUN-TEST/s, 'proxy fun');
 like(http_get('/test_var'), qr/P-TEST/s, 'foo.bar.p');
 
+$t->stop();
+
+my $content = $t->read_file('error.log');
+my $count = () = $content =~ m/js vm init/g;
+ok($count == 4, 'uniq js vm contexts');
+
 ###############################################################################
diff --git a/nginx/t/js_merge_location_blocks.t b/nginx/t/js_merge_location_blocks.t
@@ -0,0 +1,83 @@
+#!/usr/bin/perl
+
+# (C) Dmitry Volyntsev
+# (c) Nginx, Inc.
+
+# Tests for http njs module, check for proper location blocks merging.
+
+###############################################################################
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+
+###############################################################################
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+my $t = Test::Nginx->new()->has(qw/http/)
+	->write_file_expand('nginx.conf', <<'EOF');
+
+%%TEST_GLOBALS%%
+
+daemon off;
+
+events {
+}
+
+http {
+    %%TEST_GLOBALS_HTTP%%
+
+    js_import main.js;
+
+    server {
+        listen       127.0.0.1:8080;
+        server_name  localhost;
+
+        location /a {
+            js_content main.version;
+        }
+
+        location /b {
+            js_content main.version;
+        }
+
+        location /c {
+            js_content main.version;
+        }
+
+        location /d {
+            js_content main.version;
+        }
+    }
+}
+
+EOF
+
+$t->write_file('main.js', <<EOF);
+    function version(r) {
+        r.return(200, njs.version);
+    }
+
+    export default {version};
+
+EOF
+
+$t->try_run('no njs available')->plan(1);
+
+###############################################################################
+
+$t->stop();
+
+my $content = $t->read_file('error.log');
+my $count = () = $content =~ m/ js vm init/g;
+ok($count == 1, 'http js block imported once');
+
+###############################################################################
diff --git a/nginx/t/js_merge_server_blocks.t b/nginx/t/js_merge_server_blocks.t
@@ -0,0 +1,78 @@
+#!/usr/bin/perl
+
+# (C) Dmitry Volyntsev
+# (c) Nginx, Inc.
+
+# Tests for http njs module, check for proper server blocks merging.
+
+###############################################################################
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+
+###############################################################################
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+my $t = Test::Nginx->new()->has(qw/http/)
+	->write_file_expand('nginx.conf', <<'EOF');
+
+%%TEST_GLOBALS%%
+
+daemon off;
+
+events {
+}
+
+http {
+    %%TEST_GLOBALS_HTTP%%
+
+    js_import main.js;
+
+    server {
+        listen       127.0.0.1:8080;
+    }
+
+    server {
+        listen       127.0.0.1:8081;
+    }
+
+    server {
+        listen       127.0.0.1:8082;
+    }
+
+    server {
+        listen       127.0.0.1:8083;
+    }
+}
+
+EOF
+
+$t->write_file('main.js', <<EOF);
+    function version(r) {
+        r.return(200, njs.version);
+    }
+
+    export default {version};
+
+EOF
+
+$t->try_run('no njs available')->plan(1);
+
+###############################################################################
+
+$t->stop();
+
+my $content = $t->read_file('error.log');
+my $count = () = $content =~ m/ js vm init/g;
+ok($count == 1, 'http js block imported once');
+
+###############################################################################
diff --git a/nginx/t/stream_js.t b/nginx/t/stream_js.t
@@ -394,7 +394,7 @@ $t->write_file('test.js', <<EOF);
 EOF
 
 $t->run_daemon(\&stream_daemon, port(8090));
-$t->try_run('no stream njs available')->plan(24);
+$t->try_run('no stream njs available')->plan(25);
 $t->waitforsocket('127.0.0.1:' . port(8090));
 
 ###############################################################################
@@ -450,6 +450,10 @@ like($t->read_file('status.log'), qr/$p[0]:200/, 'status undecided');
 like($t->read_file('status.log'), qr/$p[1]:200/, 'status allow');
 like($t->read_file('status.log'), qr/$p[2]:403/, 'status deny');
 
+my $content = $t->read_file('error.log');
+my $count = () = $content =~ m/ js vm init/g;
+ok($count == 2, 'http and stream js blocks imported once each');
+
 ###############################################################################
 
 sub has_version {