Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: support of non-preemptive time slicing when running CPU intensive Lua code. #1052

Closed
wants to merge 3 commits into from

Conversation

dndx
Copy link
Member

@dndx dndx commented Apr 24, 2017

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

Note that the delayed field is not being used by the coctx->sleep event and we reuse it for keeping track of whether at least one event loop has passed.

Need to be used with: openresty/openresty#252

Test config:

location = / { 
    root   html;
    index  index.html index.htm;
    content_by_lua_block {
        local t = ngx.thread.spawn(function()
            for i=1,500000000 do
                if i % 100000 == 0 then
                    ngx.sleep(0)
                end 

                a = math.random()
            end 
        end)
        for i=1,500000000 do
            if i % 100000 == 0 then
                ngx.sleep(0)
            end 

            if i == 100000000 then
                ngx.thread.kill(t)
            end 

            a = math.random()
        end 
        ngx.say('it works!')
    }
}

Before the patch, curl / in one terminal while curl /404 in another window, /404 will not return until / finished.

After the patch, curl /404 is always responsible even if / is running at the same time.

ngx_add_timer(&coctx->sleep, (ngx_msec_t) delay);
coctx->sleep.delayed = 1;
ngx_post_event(&coctx->sleep, &ngx_posted_delayed_events);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: need a blank line before this } else { line.


ngx_add_timer(&coctx->sleep, (ngx_msec_t) delay);
coctx->sleep.delayed = 1;
ngx_post_event(&coctx->sleep, &ngx_posted_delayed_events);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this does not present in older nginx cores like 1.11.2 and 1.9.15? Better protect it with macros?

@dndx dndx force-pushed the cpu_yield branch 4 times, most recently from e5032b5 to 589db0f Compare April 25, 2017 00:00
@yourenyouyu
Copy link

yourenyouyu commented Apr 25, 2017

This solution does not seem to work
worker_processes 1;

		location /lua {
			default_type text/html;
			
			content_by_lua_block {
			while true do
				ngx.sleep(0);
			end

			ngx.say("hhh")
			}
		}
		
		location /test {
			default_type text/html;
			
			content_by_lua_block {
			ngx.say("hhh")
			}
		}

@dndx
Copy link
Member Author

dndx commented Apr 25, 2017

@yourenyouyu Your example works fine for me:

[datong@localhost openresty-build]$ curl 127.0.0.1:8000/lua
# stuck here

# on another window
$ curl 127.0.0.1:8000/test
hhh
$ ps aux
datong    44552  0.0  0.0  52664   856 ?        Ss   22:42   0:00 nginx: master process ./nginx/sbin/nginx
datong    44553 89.6  0.1  53056  4052 ?        R    22:42   1:01 nginx: worker process

Are you sure you have applied the NGINX core patch as well?

@yourenyouyu
Copy link

Whether your configuration is only one worker

@dndx
Copy link
Member Author

dndx commented Apr 25, 2017

@yourenyouyu it is single worker.

@dndx
Copy link
Member Author

dndx commented Apr 25, 2017

@yourenyouyu if it still does not work for you after patching, recompile using:

./configure --prefix=/home/datong/openresty-build -j4 --with-debug --with-cc-opt='-DDDEBUG=1'

and send me the NGINX error.log file so I can take a look.

@yourenyouyu
Copy link

Ok, wait a little @dndx

@dndx
Copy link
Member Author

dndx commented Apr 25, 2017

@yourenyouyu

[datong@localhost nginx-1.11.2]$ pwd
/home/datong/openresty-1.11.2.3/bundle/nginx-1.11.2
[datong@localhost nginx-1.11.2]$ curl https://raw.githubusercontent.com/dndx/openresty/86a9f38973c48ab2336af2288ce4debd8acf5e5c/patches/nginx-1.11.2-delayed_posted_events.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3088  100  3088    0     0  16905      0 --:--:-- --:--:-- --:--:-- 16874
patching file src/event/ngx_event.c
patching file src/event/ngx_event_posted.c
patching file src/event/ngx_event_posted.h

@yourenyouyu
Copy link

yourenyouyu commented Apr 25, 2017

@dndx ok, Thank you very much。Can you teach me how to use it, give me a detail step?

dd("adding timer with delay %lu ms, r:%.*s", (unsigned long) delay,
(int) r->uri.len, r->uri.data);

ngx_add_timer(&coctx->sleep, (ngx_msec_t) delay);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd better do not duplicate this code path for delay > 0. It is duplicated below in the #else branch.

I think we still need to handle the case delay == 0 when the patch is absent. In this case, we should emit an error log message so that the user can be notified about the lack of nginx core patch instead of silently ignoring it.

@agentzh
Copy link
Member

agentzh commented May 15, 2017

@dndx I wonder if it's feasible to add some tests to cover this change?

@agentzh
Copy link
Member

agentzh commented May 15, 2017

@dndx How about the following test case?

=== TEST 16: sleep 0
--- config
    location /t {
        content_by_lua_block {
            local function f (n)
                ngx.say("begin ", n)
                ngx.sleep(0)
                ngx.say("middle ", n)
                ngx.sleep(0)
                ngx.say("end ", n)
            end

            for i = 1, 3 do
                assert(ngx.thread.spawn(f, i))
            end
        }
    }
--- request
GET /t
--- response_body
begin 1
begin 2
begin 3
middle 1
middle 2
middle 3
end 1
end 2
end 3
--- no_error_log
[error]

@dndx
Copy link
Member Author

dndx commented May 15, 2017

@agentzh yes that is a good test, although it may not work without the core patch. Do you want me to add it to the PR nevertheless?

@agentzh
Copy link
Member

agentzh commented May 15, 2017

@dndx Okay, merged with the following extra patch. Thanks!

diff --git a/t/077-sleep.t b/t/077-sleep.t
index 7d295c2b..96f04bd9 100644
--- a/t/077-sleep.t
+++ b/t/077-sleep.t
@@ -9,10 +9,10 @@ log_level('debug');
 
 repeat_each(2);
 
-plan tests => repeat_each() * 63;
+plan tests => repeat_each() * 71;
 
 #no_diff();
-#no_long_string();
+no_long_string();
 run_tests();
 
 __DATA__
@@ -404,3 +404,99 @@ ok
 --- no_error_log
 [error]
 [alert]
+
+
+
+=== TEST 16: sleep 0
+--- config
+    location /t {
+        content_by_lua_block {
+            local function f (n)
+                print("f begin ", n)
+                ngx.sleep(0)
+                print("f middle ", n)
+                ngx.sleep(0)
+                print("f end ", n)
+                ngx.sleep(0)
+            end
+
+            for i = 1, 3 do
+                assert(ngx.thread.spawn(f, i))
+            end
+
+            ngx.say("ok")
+        }
+    }
+--- request
+GET /t
+--- response_body
+ok
+--- no_error_log
+[error]
+--- grep_error_log eval: qr/\bf (?:begin|middle|end)\b|\bworker cycle$|\be?poll timer: \d+$/
+--- grep_error_log_out eval
+qr/f begin
+f begin
+f begin
+worker cycle
+e?poll timer: 0
+f middle
+f middle
+f middle
+worker cycle
+e?poll timer: 0
+f end
+f end
+f end
+worker cycle
+e?poll timer: 0
+/
+
+
+
+=== TEST 17: sleep short times less than 1ms
+--- config
+    location /t {
+        content_by_lua_block {
+            local delay = 0.0005
+
+            local function f (n)
+                print("f begin ", n)
+                ngx.sleep(delay)
+                print("f middle ", n)
+                ngx.sleep(delay)
+                print("f end ", n)
+                ngx.sleep(delay)
+            end
+
+            for i = 1, 3 do
+                assert(ngx.thread.spawn(f, i))
+            end
+
+            ngx.say("ok")
+        }
+    }
+--- request
+GET /t
+--- response_body
+ok
+--- no_error_log
+[error]
+--- grep_error_log eval: qr/\bf (?:begin|middle|end)\b|\bworker cycle$|\be?poll timer: \d+$/
+--- grep_error_log_out eval
+qr/f begin
+f begin
+f begin
+worker cycle
+e?poll timer: 0
+f middle
+f middle
+f middle
+worker cycle
+e?poll timer: 0
+f end
+f end
+f end
+worker cycle
+e?poll timer: 0
+/

@agentzh agentzh closed this May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants