Njs: WIP: Refactored function call.

[hongzhidao]

Hi @xeioex

The patch is work-in-process.

I'll try to make the codes related to function call more clear.
Take a look, please.

https://gist.github.com/hongzhidao/8d5a89a5a5587cb6781d9aa9a411954b
https://gist.github.com/hongzhidao/0a07d7af0bf92c5de793b1475b6117c7

There are some thoughts.

1. await, async
   I hope njs can support the following way. Some developers dont like the way of callback.

var res = subrequest('/some'); -- keywords: asynchronize, and yield, resume...
console.log(res);

I think we can implement it by njs_continuation_t.
We know there are two keys await, async in JS language, but it seems not be neccessay in njs.
But it's very helpful if developers can use the similar way in njs such above example.
The only thing we should do is external method can be yield and be resume by njs_vm_start.
Lua module do it like this.

1. njs_continuation_t can also be used in function not only in native function.
   But I still try to implement the first though, it's not ready yet.

2. The most important thing is the basic codes relate to function call need to be refactored.

[xeioex]

hi @hongzhidao,

Some developers dont like the way of callback.

I agree that callbacks are inconvenient. And there are plans to support async, await in the future.

But, no less important goal is JS specs conformance (for example nis should be able to pass test262. By the way, the current status is 23% (6448 of 28065 tests, ~half of the fails are about >= ES6 features)). We want to run conforming js code with minimal or without any changes at all. It would allow us to load existing js modules.

In JS, promises is a way to cope with the callback hell. AFAIK, await, async are simply a syntax sugar around it. So, the plan is to support native promises first. After it, async and await can be implemented.

[xeioex]

@hongzhidao
Here is the way, you can run test262 with njs, yourself: https://gist.github.com/xeioex/3cd6b4a5bd4ed856b753645b063ae282

[hongzhidao]

I prefer to keep njs minimal. In real case await, async is just sugar, even we don't use them at all in nginx config.
But we can extend the language like external variables and method that have been implemented.
The important feature we should support is the way of using sync instead of callback.
So I think if we can yield and resume external function in nginx module (Lua do it this way), it will work perfectly.

[hongzhidao]

My thought is njs doesn't support async operations, but keep the hook (njs_continuation_t),
the async event happens and be resolved in ngx_http_js_module. (In ngx_http_js_ext_subrequest, etc).

[xeioex]

I like callbacks, because they are flexible and align well with nginx itself (you can execute several subrequests in parallel, for example). I don't like them because they are cumbersome and ugly when you have more than 2 of them. In my view, promises and async await is a way to handle callbacks (JS way).

So I think if we can yield and resume external function in nginx module (Lua do it this way), it will work perfectly.

I am afraid, this will not be javascript anymore. the issue with mixing sync code with the existing js code it that it will not work as expected. Ideally, we want to reuse as much existing js code
as possible (so, people can copy-paste some simple snippets written somewhere else).

My thought is njs doesn't support async operations, but keep the hook (njs_continuation_t),
the async event happens and be resolved in ngx_http_js_module. (In ngx_http_js_ext_subrequest, etc).

For example, think of multiple parallel subrequests with several setTimeout(). For additional examples we want to support see https://github.com/nginx/njs/issues/66.

I hope, this is not going to be a huge problem, because:
In the limit, people will be able to write almost linear code using async, await. At the same time, callbacks will also be available.

[hongzhidao]

OK.

1. I guess the way of writing linear code would be welcome.
   Many developers used to with Lua(nginx-lua-module), and it's really convenient.
2. Do you consider the patch of refactoring function call? Maybe do more.

[xeioex]

Do you consider the patch of refactoring function call? Maybe do more.

yes. I am looking into it.

[hongzhidao]

BTW, see the structure.

struct njs_function_s {
    njs_object_t                      object;

    uint8_t                           args_types[NJS_ARGS_TYPES_MAX];
    uint8_t                           args_offset;
    uint8_t                           continuation_size;

The field continuation_size belongs to njs_function_t.
I think it'll be interesting that hook continuation in njs_function_call (or njs_vmcode_interpreter).
I'll try to implement it without breaking JS specs conformance as an experiment.

I still think njs_continuation_nexus can be used in function and native_function.

[xeioex]

@hongzhidao,

Do you consider the patch of refactoring function call?

I like njs_function_native_call() function, except the successful return value sizeof(njs_vmcode_function_call_t) (which is only applicable in njs_vm.c code). Will fix it. Sent patches to @igorsysoev for approve.

[xeioex]

@hongzhidao,

my improvements over: https://gist.github.com/xeioex/a0da47724c4066cf118b6bb756323457

[hongzhidao]

deleted

[hongzhidao]

Hi @xeioex

I rearranged these patches relate to function call.

1. Fixed vmcode function call.
2. Refactored njs_function_frame_free.
3. Introduced function native call.
4. Move calling njs_normalize_args into njs_function_native_call.
5. Introduced njs_function_frame_activate.
6. Replace njs_function_frame_activate as njs_function_activate.
7. Style.

https://gist.github.com/hongzhidao/f08900f0be20ace3f7c25f97801c9104

[hongzhidao]

It looks like a bug. We should always recover the current pointer after calling njs_vm_call .

diff -r 14f1b7a9748b njs/njs.c
--- a/njs/njs.c Tue Jan 08 05:02:20 2019 +0800
+++ b/njs/njs.c Tue Jan 08 05:08:21 2019 +0800
@@ -476,11 +476,13 @@
                                 sizeof(njs_vmcode_generic_t));

     if (nxt_slow_path(ret == NXT_ERROR)) {
-        return ret;
-     }
+        goto done;
+    }

     ret = njs_vm_start(vm);

+done:
+
     vm->current = current;

     return ret;

[xeioex]

@hongzhidao

Move calling njs_normalize_args into njs_function_native_call.

Style fixes: https://gist.github.com/xeioex/2b419784305db55fe0aceef30b9cfa7e

[xeioex]

@hongzhidao

Introduced njs_function_frame_activate.

Style: https://gist.github.com/908ac20035b36bc29d230f7a7fe1df42

[xeioex]

https://gist.github.com/hongzhidao/f08900f0be20ace3f7c25f97801c9104

Thank you for contribution. The patch series is under review. It may take a while to be committed (@igorsysoev is on vacation until tomorrow).

[hongzhidao]

Hi @xeioex

1. I append a patch in the patch series.
   https://gist.github.com/hongzhidao/f08900f0be20ace3f7c25f97801c9104
   Fixed njs_vm_call.
   Refactored njs_function_apply.
   Style.

2. Optional patch.
   https://gist.github.com/hongzhidao/c17a3a016c3db119b25210d7a16b5232
   Introducted njs_function_lambda_xxx.
   Introduced njs_function_call.

3. It seems continuation can be refactored. It's not ready yet.

Previously, the default meaning of function is lambda, such as njs_function_frame, njs_function_call. But it still causes confusion as codes become more.

The ideal state is there are only three models in njs public APIs.

1. njs_function_activate/njs_function_apply: These generate frame and put them in vm codes.
2. njs_function_frame(njs_function_lambda_frame, njs_function_native_frame).
3. njs_function_call(njs_function_lambda_call, njs_function_native_call).
4. Optionally continuaction is also be refactored, but I'm not sure now.

Thanks.

[drsm]

Hi!

1. I guess the way of writing linear code would be welcome.
   Many developers used to with Lua(nginx-lua-module), and it's really convenient.

@hongzhidao, here is another simple example of callbacks vs promises vs async/await https://github.com/nginx/njs/issues/31#issuecomment-402798056

[hongzhidao]

Hi @drsm , thank you.
Actually, I agree with xieoex's view. On the other hand, the ideal is that developers don't need to consider which type to choose (async or sync).
And look into the implemetation, I think if the vm codes and be yielded and resumed, actually it does now, we can implement the way of writing linear code like lua. It needn't to break the JS specs conformance.
Anyway, it's a good news that async/await will be implemented in the future.

[xeioex]

@hongzhidao Thank you, patch series committed.

[hongzhidao]

Hi @xeioex

Fixed a introduced bug. It happends on ngx_http_js_module.

 710     if (njs_vm_call(ctx->vm, func, njs_value_arg(ctx->args), 2) != NJS_OK

Patch.

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1547146152 -28800
# Node ID 171199d61f0dd6358e99f5ce058b9ccfc7d1cebd
# Parent  112bd230858c55ec02fbfebcd07f2f7d7fda12a9
Fixed njs_function_activate.

diff -r 112bd230858c -r 171199d61f0d njs/njs_function.c
--- a/njs/njs_function.c    Thu Jan 10 20:18:27 2019 +0300
+++ b/njs/njs_function.c    Fri Jan 11 02:49:12 2019 +0800
@@ -492,7 +492,7 @@

     vm->active_frame = frame;

-    return NJS_APPLIED;
+    return NXT_OK;
 }


@@ -966,7 +966,7 @@
         cont->return_address = vm->current + advance;
         vm->current = (u_char *) njs_continuation_nexus;

-        return NJS_APPLIED;
+        return NXT_OK;
     }

     ret = njs_function_lambda_frame(vm, function, this, args, nargs, 0);
diff -r 112bd230858c -r 171199d61f0d njs/njs_function.h
--- a/njs/njs_function.h    Thu Jan 10 20:18:27 2019 +0300
+++ b/njs/njs_function.h    Fri Jan 11 02:49:12 2019 +0800
@@ -197,8 +197,12 @@
 njs_function_apply(njs_vm_t *vm, njs_function_t *function,
     const njs_value_t *args, nxt_uint_t nargs, njs_index_t retval)
 {
-    return njs_function_activate(vm, function, &args[0], &args[1], nargs - 1,
-                                 retval, 0);
+    njs_ret_t  ret;
+
+    ret = njs_function_activate(vm, function, &args[0], &args[1], nargs - 1,
+                                retval, 0);
+
+    return ret == NXT_OK ? NJS_APPLIED : NXT_ERROR;
 }


diff -r 112bd230858c -r 171199d61f0d njs/njs_vm.c
--- a/njs/njs_vm.c  Thu Jan 10 20:18:27 2019 +0300
+++ b/njs/njs_vm.c  Fri Jan 11 02:49:12 2019 +0800
@@ -2028,11 +2028,11 @@
         ret = njs_function_lambda_call(vm, (njs_index_t) retval,
                                        sizeof(njs_vmcode_function_call_t));

-        if (nxt_fast_path(ret != NJS_ERROR)) {
-            return 0;
+        if (nxt_fast_path(ret != NXT_OK)) {
+            return ret;
         }

-        return ret;
+        return 0;
     }

     args = frame->arguments;

[xeioex]

@hongzhidao,

Actually, I committed a bit different patch
https://github.com/nginx/njs/commit/011d465dbc7d1c67f17db3be03bac5310eece047

So, NJS_APPLIED is handled inside njs_vm_call().

[hongzhidao]

deleted.