Njs: Synchronous version of njs_primitive_value() and njs_value_property().

[xeioex]

[xeioex]

@hongzhidao

Early alpha: https://gist.github.com/7a1bea4c1b623dbc9cdf4e7fe59f9d68
make test passes.

The most important changes:

1) njs_primitive_value() -> njs_value_to_primitive()
njs_value_to_primitive() works synchronously because of njs_function_call().

2) traps and continuations can be removed (done for njs_normalize_args()).

What is left:

1) cleanup remaining njs_trap() (and rewrite remaining instructions like njs_vmcode_increment()). add unit tests for remaining instructions (not covered now by unit tests).
2) remove trap fields, functions, constants from vm.
3) rewrite njs_function_apply() to njs_function_call()
4) rewrite functions with continuations.

1) simplifications after refactoring.

[hongzhidao]

@xeioex take a look, please.

1. Introduced njs_value.h/c files.
   https://gist.github.com/hongzhidao/bec440d5cad8bb8d7f66c21d98dade6b
   I prefer to make njs_vm.c only do the thing with regard to vm codes, and njs_value is an important model, we can separate it from njs_vm.

2. On reviewing the patch synchronous.... But the file njs_vm.c is kind of large.
   2.1. Noticed that vm->retval.type = object->type; is not always NJS_OBJECT.
   https://gist.github.com/xeioex/7a1bea4c1b623dbc9cdf4e7fe59f9d68#file-github190-patch-L190
   2.2. renaming argument value as src. Like below.

njs_ret_t njs_value_to_primitive(njs_vm_t *vm, njs_value_t *dst, njs_value_t *src, nxt_uint_t hint);
njs_ret_t njs_value_to_numeric(njs_vm_t *vm, njs_value_t *dst, njs_value_t *src);
njs_ret_t njs_value_to_string(njs_vm_t *vm, njs_value_t *dst, njs_value_t *src);

2.3. It seems unnecessary.
https://gist.github.com/xeioex/7a1bea4c1b623dbc9cdf4e7fe59f9d68#file-github190-patch-L353
2.4. Why not keep it?
https://gist.github.com/xeioex/7a1bea4c1b623dbc9cdf4e7fe59f9d68#file-github190-patch-L485
I mean if the arg is undefined, it'll be changed into number after the patch. But in njs_array_prototype_slice_continuation

 572         if (!njs_is_undefined(njs_arg(args, nargs, 2))) {
 573             end = njs_primitive_value_to_integer(&args[2]);
 574
 575         } else {
 576             end = length;
 577         }

[hongzhidao]

@xeioex @drsm

new 0[undefined]

TypeError: (intermediate value)[""] is not a function
at main (native)

It's the same as this.
https://gist.github.com/xeioex/7a1bea4c1b623dbc9cdf4e7fe59f9d68#file-github190-patch-L1744

I think it's an issue. On it.

[hongzhidao]

@xeioex

My thoughts.

1. Introduce njs_value files first.
2. Add njs_date and some more macros into njs_value.
3. Fixed 0[undefined], it may change njs_property_query.
4. Try to eliminate NJS_TRAP_XXX by introducing njs_value_to_xxx, but keep these functions njs_primitive_value currently, it'll be a handful to review.

[hongzhidao]

@xeioex

TypeError: (intermediate value)[""] is not a function
at main (native)
It's the same as this.
https://gist.github.com/xeioex/7a1bea4c1b623dbc9cdf4e7fe59f9d68#file-github190-patch-L1744

diff -r 67481ba6d380 njs/njs_vm.c
--- a/njs/njs_vm.c  Fri Jul 05 21:45:28 2019 +0300
+++ b/njs/njs_vm.c  Sun Jul 07 12:06:53 2019 -0400
@@ -2125,6 +2125,12 @@ njs_vmcode_method_frame(njs_vm_t *vm, nj
     }

     if (value == NULL || !njs_is_function(value)) {
+
+        ret = njs_value_to_string(vm, name, name);
+        if (nxt_slow_path(ret != NXT_OK)) {
+            return NXT_ERROR;
+        }
+
         njs_string_get(name, &string);
         njs_type_error(vm, "(intermediate value)[\"%V\"] is not a function",
                        &string);
diff -r 67481ba6d380 njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c  Fri Jul 05 21:45:28 2019 +0300
+++ b/njs/test/njs_unit_test.c  Sun Jul 07 12:06:53 2019 -0400
@@ -1645,9 +1645,8 @@ static njs_unit_test_t  njs_test[] =
     { nxt_string("'-1' < {valueOf: function() {return -2}}"),
       nxt_string("false") },

-    // FIXME
-    //{ nxt_string("new 0[isNaN]"),
-    //nxt_string("TypeError: (intermediate value)[\"[object Function]\"] is not a function") },
+    { nxt_string("new 0[isNaN]"),
+    nxt_string("TypeError: (intermediate value)[\"[object Function]\"] is not a function") },

     /**/

1. new 0[undefined]
   Fixed this after introducing njs_value?
2. Help to add unit test for it.

[hongzhidao]

@xeioex

traps and continuations can be removed (done for njs_normalize_args()).

I think continuation is a good design. It's clear in many places. Such as:

typedef struct {
    union {
        njs_continuation_t  cont;
        u_char              padding[NJS_CONTINUATION_SIZE];
    } u;
    /*
     * This retval value must be aligned so the continuation is padded
     * to aligned size.
     */
    njs_value_t             length;
} njs_array_slice_t;

The bad things are trap and NJS_APPLY, especially NJS_APPLY. it concerns to njs_function_apply, etc.

And consider this.

As I understand, continunations are applied to the same frame. In synchronous version of njs_function_call, it only needs one retval and return_address.

[xeioex]

@hongzhidao

I think continuation is a good design. It's clear in many places.

Can you provide an example? If we can do all the conversions (without NJS_TRAP, NJS_APPLY) in place what is the point for continuations? Instead we can write simple native functions.

[xeioex]

@hongzhidao

Introduce njs_value files first.
Add njs_date and some more macros into njs_value.
Fixed 0[undefined], it may change njs_property_query.

Agree, will do.

On reviewing the patch synchronous.... But the file njs_vm.c is kind of large.

thanks for catching these bugs, will apply.

[hongzhidao]

Can you provide an example? If we can do all the conversions (without NJS_TRAP, NJS_APPLY) in place what is the point for continuations? Instead we can write simple native functions.

No problem, it'll be better if we can remove continuation.

[xeioex]

Current version: https://gist.github.com/xeioex/ce6ea615e367f706ac7f7f941ba398fc

[hongzhidao]

@xeioex

Now, the current version does not concern continuation.
Do you have a clear thought of how to remove continuation?
For example, how to process the below method?

{
        .type = NJS_METHOD,
        .name = njs_string("slice"),
        .value = njs_native_function(njs_array_prototype_slice,
                     njs_continuation_size(njs_array_slice_t),
                     NJS_OBJECT_ARG, NJS_INTEGER_ARG, NJS_INTEGER_ARG),
        .writable = 1,
        .configurable = 1,
    },

BTW, I think only the native function in njs_continuation_t is needed. So it's not hard to eliminate njs_continuation_t.

typedef struct {
    njs_function_native_t          function;
    uint8_t                        *args_types;
    u_char                         *return_address;
    njs_index_t                    retval;
} njs_continuation_t;

I think removing continuation is more critical than njs_normalize_args because many functions are called by njs_continuation_nexus. I am also trying to do this now. But it's not easy.

[xeioex]

@hongzhidao

Now, the current version does not concern continuation.

yes, this is still in progress, just the updated patch after splitting other patches.

Do you have a clear thought of how to remove continuation?
For example, how to process the below method?

yes, once njs_value_property() does not throw NJS_APPLY and

njs_vm_trap_value(vm, &slice->length);
return njs_trap(vm, NJS_TRAP_NUMBER_ARG);

is replaced with njs_value_to_numeric() continuation can be removed for the function.

I think removing continuation is more critical than njs_normalize_args because many functions are called by njs_continuation_nexus

once all functions are rewritten, njs_continuation_nexus can also be removed.

IMO continuation is a way to preserve state of the function which can be interrupted by NJS_APPLIED and NJS_TRAP calls. If these are eliminated continuations can be eliminated as well.

[hongzhidao]

@xeioex

Have you processed njs_values_equal? I want a reference.

[xeioex]

@hongzhidao

updated: https://gist.github.com/0913878181ae007d794b290101f373f7

traps are removed completely. make test passes.
test262 46 test degradation against the upstream.

continuations are still to be fixed. I think it should be a separate patch.

[hongzhidao]

@xeioex great job.

Style.

[hongzhidao]

continuations are still to be fixed. I think it should be a separate patch.

Agree, but submit them after completely finished.

[xeioex]

@hongzhidao

regarding njs_values_to_numeric()
here we have a copy

+ *numeric1 = *val1;
+ *numeric2 = *val2;

even for the fast path (when operands are primitive), whereas in my patch there is no copy (32 bytes in total).
If you still think that you variant is better, please provide a benchmark. We can also compare assembler code for these instructions.

[hongzhidao]

@xeioex

benchmark.js

var i;

function test() {
    console.time();
    function sub() {
        10000 - 1000;
    }

    for (i = 0; i < 5000000; ++i) {
        sub();
    }
    console.timeEnd();
}

for (var j = 0; j < 5; ++j) {
    test();
    console.log('\n');
}

1. version without diff.patch

default: 363.175600ms
default: 360.910170ms
default: 365.394131ms
default: 348.662729ms
default: 355.265518ms

1. version with diff.patch

default: 382.959125ms
default: 412.826239ms
default: 389.060190ms
default: 389.979454ms
default: 393.934276ms

1. version with diff2.patch

default: 356.613865ms
default: 344.740873ms
default: 348.726387ms
default: 368.939255ms
default: 351.692535ms

https://gist.github.com/hongzhidao/3061f59d68398e8e89ff94c5dc0fbfd7

[xeioex]

@hongzhidao

updated: https://gist.github.com/95da20c709bd6b9b42321b61f1ac4b00

 === Summary ===
  - Ran 27119 tests
- - Passed 8227 tests (30.3%)
- - Failed 18892 tests (69.7%)
+ - Passed 8209 tests (30.3%)
+ - Failed 18910 tests (69.7%

[hongzhidao]

@xeioex

With synchronous version, these will be?

1. rewritten njs_vmcode_function_call.

njs_vmcode_function_call(njs_vm_t *vm, njs_value_t *invld, njs_value_t *retval) {
    ...  // sync calling, may just call njs_function_call.

    return sizeof(njs_vmcode_function_call_t);
}

1. njs_function_activate will be removed, there is only one function doing the calling, it's njs_function_call.
2. all the codes related to continuations will be removed?
3. all the structures related to continuation will be rewritten? Such as.

typedef struct {
    union {
        njs_continuation_t  cont;
        u_char              padding[NJS_CONTINUATION_SIZE];
    } u;
    /*
     * This retval value must be aligned so the continuation is padded
     * to aligned size.
     */
    njs_value_t             length;
} njs_array_slice_t;

[xeioex]

@hongzhidao

With synchronous version, these will be?

1. ..
2. ..
3. ..
4. ..

yes

[hongzhidao]

@xeioex

I'm courious how you will process the case of njs_array_prototype_for_each?

var a = [1,2,3];
var s = { sum: 0 };
a.forEach(function(v, i, a) { this.sum += v }, s);
s.sum

[xeioex]

updated: https://gist.github.com/020b17e9b2053e35604a8dd13d06d7cc

[xeioex]

@hongzhidao

njs_array_prototype_for_each

njs_array_prototype_for_each will be rewritten with a simple loop over an array.|

njs_array_iterator_apply() will be replaced with njs_function_call().

[hongzhidao]

@xeioex

njs_array_prototype_for_each will be rewritten with a simple loop over an array.|
njs_array_iterator_apply() will be replaced with njs_function_call().

Pretty good.

diff -r bb0ed7b906bc njs/njs_array.c
--- a/njs/njs_array.c   Fri Jul 05 21:45:28 2019 +0300
+++ b/njs/njs_array.c   Thu Jul 11 23:46:35 2019 -0400
@@ -104,8 +104,6 @@ static njs_ret_t njs_array_prototype_fil
     njs_value_t *args, nxt_uint_t nargs, njs_index_t unused);
 static njs_ret_t njs_array_prototype_fill_object_continuation(njs_vm_t *vm,
     njs_value_t *args, nxt_uint_t nargs, njs_index_t unused);
-static njs_ret_t njs_array_prototype_for_each_continuation(njs_vm_t *vm,
-    njs_value_t *args, nxt_uint_t nargs, njs_index_t unused);
 static njs_ret_t njs_array_prototype_some_continuation(njs_vm_t *vm,
     njs_value_t *args, nxt_uint_t nargs, njs_index_t unused);
 static njs_ret_t njs_array_prototype_every_continuation(njs_vm_t *vm,
@@ -1564,38 +1562,40 @@ static njs_ret_t
 njs_array_prototype_for_each(njs_vm_t *vm, njs_value_t *args, nxt_uint_t nargs,
     njs_index_t unused)
 {
-    nxt_int_t         ret;
-    njs_array_iter_t  *iter;
-
-    ret = njs_array_iterator_args(vm, args, nargs);
-    if (nxt_slow_path(ret != NXT_OK)) {
-        return ret;
+    uint32_t           i, length;
+    nxt_int_t          ret;
+    njs_value_t        *value, arguments[4];
+    const njs_value_t  *this;
+
+    if (nargs < 2 || !njs_is_array(&args[0]) || !njs_is_function(&args[1])) {
+        njs_type_error(vm, "unexpected iterator arguments");
+        return NXT_ERROR;
     }

-    iter = njs_vm_continuation(vm);
-    iter->u.cont.function = njs_array_prototype_for_each_continuation;
-
-    return njs_array_prototype_for_each_continuation(vm, args, nargs, unused);
-}
-
-
-static njs_ret_t
-njs_array_prototype_for_each_continuation(njs_vm_t *vm, njs_value_t *args,
-    nxt_uint_t nargs, njs_index_t unused)
-{
-    uint32_t          index;
-    njs_array_iter_t  *iter;
-
-    iter = njs_vm_continuation(vm);
-
-    index = njs_array_iterator_index(njs_array(&args[0]), iter);
-
-    if (index == NJS_ARRAY_INVALID_INDEX) {
-        vm->retval = njs_value_undefined;
-        return NXT_OK;
+    this = njs_arg(args, nargs, 2);
+    length = njs_array_len(&args[0]);
+
+    for (i = 0; i < length; i++) {
+        value = &njs_array_start(&args[0])[i];
+
+        if (njs_is_valid(value)) {
+            arguments[0] = *this;
+            arguments[1] = *value;
+            njs_set_number(&arguments[2], i);
+            arguments[3] = args[0];
+
+            ret = njs_function_call(vm, njs_function(&args[1]), arguments, 4,
+                                    (njs_index_t) &vm->retval);
+
+            if (nxt_slow_path(ret != NXT_OK)) {
+                return ret;
+            }
+        }
     }

-    return njs_array_iterator_apply(vm, iter, args, nargs);
+    vm->retval = njs_value_undefined;
+
+    return NXT_OK;
 }

Tell me what I can help with the patch if needed :)

[xeioex]

@hongzhidao

Tell me what I can help with the patch if needed :)

you can help by rewriting all the continuation functions from njs_array.c over the current patch.
I will continue with removing continuation infrastructure.
Currently on some 32bit issues with the current patch on some platforms.

[hongzhidao]

OK.

[hongzhidao]

@xeioex

Are you on njs_value_property?
A few continuation functions depend on it.

you can help by rewriting all the continuation functions from njs_array.c over the current patch.

I'm on these continuation functions in njs_array.c except those depend on njs_value_property.

[xeioex]

@hongzhidao

not yet.

[hongzhidao]

@xeioex

There are so many places need to be changed.

1. Here are a few functions passed based on your current patch.
   https://gist.github.com/hongzhidao/8372f6a67fd91e3e8f26d767db104979
   While I refactored other continuation functions in njs_array.c, I found they depend on those core functions: njs_value_property, njs_function_activate, and others may return NJS_APPLIED functions.
   Then I try to implement the sync version of njs_function_lambda_call. I think njs_vmcode_run(vm) can be moved into njs_function_lambda_call. (Since array tests passed.)

1. Here is the incomplete patch. (array tests passed, others like string tests are broken.)
   https://gist.github.com/hongzhidao/6c71119ff1fb0c96b71dd5f0ec130a98
   We may be doing some of the same things.

[xeioex]

@hongzhidao

Here is the incomplete patch. (array tests passed, others like string tests are broken.)

Looks promising.

We may be doing some of the same things.

I have not started yet.

[hongzhidao]

Just updated.
https://gist.github.com/hongzhidao/24999970a81606511f6b2a5cec39838a

1. Remove all continuation functions in njs_array.c and njs_string.c. Some tests are broken now.
2. On removing NJS_APPLIED. Function prototype functions "call/apply" need to be refactored first.

[hongzhidao]

@xeioex

Updated.
https://gist.github.com/hongzhidao/0e589ac05865da5861aba096ee74e8b1

1. continuation is completely removed.
   These files include continuation functions,
   they are njs_array.c, njs_string.c, njs_json.c, njs_fs.c, njs_regex.c and njs_date.c.
2. NJS_APPLIED is close to being removed.
3. we need another function similar njs_function_call, it supports this argument.
   See njs_function_call2.
4. njs_function_apply, njs_function_activate are moved.

What I'm not sure.
It seems that frame->call can be removed?

Now broken tests are commented, so we can do the left things by fixing them.
After this, I think we need to rewrite the patch again by small steps.

[xeioex]

@hongzhidao

It seems that frame->call can be removed?

yes, I thought about it.

Now broken tests are commented, so we can do the left things by fixing them.

On it.

[hongzhidao]

@xeioex

1. njs_function_call and njs_function_apply.
   In JS, the function call declaration is function.call(thisArg, arg1, arg2, ...).
   It supports this argument.

So what about adding this argument in njs_function_call?

njs_ret_t njs_function_call(njs_vm_t *vm, njs_function_t *function, njs_value_t *this, njs_value_t *args,
    nxt_uint_t nargs, njs_index_t retval);

Then add another API njs_function_apply which does not support this argument.

njs_ret_t njs_function_apply(njs_vm_t *vm, njs_function_t *function, njs_value_t *args,
    nxt_uint_t nargs, njs_index_t retval);

1. some simplifies.

nxt_inline njs_ret_t
njs_function_frame_invoke(njs_vm_t *vm, njs_index_t retval)
{
    njs_native_frame_t  *frame;

    frame = vm->top_frame;

    if (frame->function->native) {
        return njs_function_native_call(vm, retval);

    } else {
        return njs_function_lambda_call(vm, retval);
    }
}

njs_ret_t
njs_function_native_call(njs_vm_t *vm, njs_index_t retval)
{
    njs_ret_t           ret;
    njs_value_t         *value, *args;
    njs_function_t      *function;
    njs_native_frame_t  *frame;

    frame = vm->top_frame;
    function = vm->top_frame->function;

    ret = njs_normalize_args(vm, frame->arguments, function->args_types,
                             frame->nargs);
    if (ret != NJS_OK) {
        return ret;
    }

    ret = function->u.native(vm, frame->arguments, frame->nargs, retval);

    frame = vm->top_frame;
    function = vm->top_frame->function;
    ...
}

Now broken tests are commented, so we can do the left things by fixing them.
On it.

1. Now, I'm not doing the fixing, I'm on trying to split this big patch into small patches.

[xeioex]

@hongzhidao

So what about adding this argument in njs_function_call?

Looks good.

I am still on the first patch. many 32 bit issues with njs_index_t and retvals.

[xeioex]

@hongzhidao

https://gist.github.com/hongzhidao/0e589ac05865da5861aba096ee74e8b1#file-njs-190-5-patch-L1509

BTW, while(1) {} can be easily deleted.

[hongzhidao]

@xeioex

A bit update.
https://gist.github.com/hongzhidao/dc9fb0e1c4f854536e30b7916daa5689

1. while(1) {} can be easily deleted.
2. njs_function_call(this, arguments) and njs_function_apply(arguments).
3. It seems that frame->call can be removed.
   I think njs_vmcode_return always return NJS_STOP. Now, it's called by njs_vmcode_run() in njs_function_lambda_call.
   But in njs_vmcode_interpreter, help checking whether it can be removed.

 160             if (call) {
 161                 return NXT_ERROR;
 162             }

1. Many fixes by fixing njs_function_native_call.
   Still on it.

[hongzhidao]

@xeioex

It seems frame->call and frame->skip can be eliminated, but I faced some problems.

1. In njs_vmcode_interpreter().

 160             if (call) {
 161                 return NXT_ERROR;
 162             }

1. In njs_function_native_call().

 626         if (!native->skip) {  // under what case, native can be skip here?
 627             value = njs_vmcode_operand(vm, retval);
 628             /*
 629              * GC: value external/internal++ depending
 630              * on vm->retval and retval type
 631              */
 632             *value = vm->retval;
 633         }

Need your help.

[xeioex]

@hongzhidao

// under what case, native can be skip here?

njs_function_prototype_call()
...
      /* Skip the "call" method frame. */                                                                                        
      vm->top_frame->previous->skip = 1;

BTW, I would prefer to eliminate skip also. Also consider this test
Object((new Date(0)).toJSON())
https://github.com/nginx/njs/blob/master/njs/njs_function.h#L239

[hongzhidao]

deleted.

[hongzhidao]

I still can't find a test covers this branch, can you provide an example?

 626         if (!native->skip) {  // under what case, native can be skip here?
 627             value = njs_vmcode_operand(vm, retval);
 628             /*
 629              * GC: value external/internal++ depending
 630              * on vm->retval and retval type
 631              */
 632             *value = vm->retval;
 633         }

Because it's sync now, it seems it's not unneccessary. (On it)
If yes, we can do more things in njs_function_frame_free().

    // In njs_vmcode_return() and njs_function_native_call().
    // move these codes into njs_function_frame_free.
    previous = njs_function_previous_frame(&frame->native);

    njs_vm_scopes_restore(vm, frame, previous);

    /*
     * If a retval is in a callee arguments scope it
     * must be in the previous callee arguments scope.
     */
    retval = njs_vmcode_operand(vm, frame->retval);

    /* GC: value external/internal++ depending on value and retval type */
    *retval = *value;

    njs_function_frame_free(vm, &frame->native);

[xeioex]

@hongzhidao

Can we remove native->skip completely?
If all the tests pass, we can safely remove it I think.

If Function.prototype.call and apply can be written without it.

[hongzhidao]

Can we remove native->skip completely?

Not sure now.

If all the tests pass, we can safely remove it I think.

Agree, the most important thing is making all the tests pass.

If Function.prototype.call and apply can be written without it.

It's hard to make all tests pass without changing it.
But see the current patch, it works and makes sense. (return NJS_DECLIENED, can be improved later.)
https://gist.github.com/hongzhidao/dc9fb0e1c4f854536e30b7916daa5689

[hongzhidao]

@xeioex

the most important thing is making all the tests pass.

We can do it based on the patch.
https://gist.github.com/hongzhidao/1487d59963ca12143d1558fe9cf8298b

Now, I'm just on https://gist.github.com/hongzhidao/1487d59963ca12143d1558fe9cf8298b#file-njs-190-7-patch-L4884.

WIP:
Fixed json parse. https://gist.github.com/hongzhidao/04497e4d93b05a2660da496faa66df4a#file-njs-190-8-patch-L3198
https://gist.github.com/hongzhidao/04497e4d93b05a2660da496faa66df4a

Fixed json stringify.
https://gist.github.com/hongzhidao/b82a130af49f9c4f4f28e4f5228487b2

[hongzhidao]

@xeioex

https://gist.github.com/hongzhidao/b82a130af49f9c4f4f28e4f5228487b2
Now, there a few broken tests left.

My thoughts.

1. Make all the tests passed.
2. Split the big patch into small patches.
3. Checking whether frame->call and skip can be eliminated.

[xeioex]

+#if 0
     { nxt_string("Object.defineProperty(Function.prototype, \"prototype\", {get: ()=>Array.prototype});"
                  "[] instanceof Function.prototype"),
       nxt_string("InternalError: getter is not supported in instanceof") },
+#endif

BTW, it should fail as TypeError was a workaround.

[xeioex]

@hongzhidao

Great job!

test262 status is the same as before refactoring

=== Summary ===                                                                                                                  
 - Ran 27119 tests                                                                                                               
 - Passed 8228 tests (30.3%)                                                                                                     
 - Failed 18891 tests (69.7%)

A single additional test fails https://gist.github.com/e85d0bc243f67db0ab0dbbf13450cb15

Unfortunately nginx-tests fails, I am on it (they fail on the first patch also).

Test Summary Report
-------------------
./js.t                                   (Wstat: 5888 Tests: 28 Failed: 23)
  Failed tests:  1-6, 10-12, 14-27
  Non-zero exit status: 23
./js_async.t                             (Wstat: 2048 Tests: 9 Failed: 8)
  Failed tests:  1-8
  Non-zero exit status: 8
./js_headers.t                           (Wstat: 256 Tests: 14 Failed: 1)
  Failed test:  13
  TODO passed:   3, 10-12
  Non-zero exit status: 1
./js_internal_redirect.t                 (Wstat: 256 Tests: 5 Failed: 1)
  Failed test:  4
  Non-zero exit status: 1
./js_modules.t                           (Wstat: 768 Tests: 4 Failed: 3)
  Failed tests:  1-3
  Non-zero exit status: 3
./js_paths.t                             (Wstat: 1280 Tests: 6 Failed: 5)
  Failed tests:  1-5
  Non-zero exit status: 5
./js_request_body.t                      (Wstat: 256 Tests: 5 Failed: 1)
  Failed test:  4
  TODO passed:   2
  Non-zero exit status: 1
./js_return.t                            (Wstat: 1536 Tests: 7 Failed: 6)
  Failed tests:  1-6
  Non-zero exit status: 6
./js_subrequests.t                       (Wstat: 5888 Tests: 27 Failed: 23)
  Failed tests:  1-4, 6-16, 19-26
  Non-zero exit status: 23
./stream_js.t                            (Wstat: 256 Tests: 21 Failed: 1)
  Failed test:  20
  TODO passed:   7-16
  Non-zero exit status: 1
./stream_js_variables.t                  (Wstat: 256 Tests: 4 Failed: 1)
  Failed test:  3
  TODO passed:   1-2
  Non-zero exit status: 1

[xeioex]

@hongzhidao

Split the big patch into small patches.

not sure it worth the efforts.

Additional place to improve:

- RangeError: Maximum call stack size exceeded
-    at anonymous (:124)
-      repeats 16388 times
-    at main (native)===
+ RangeError: recursion limit reached
+    at anonymous (:124)===

currently recursion is not recorded in the backtrace. But it can be fixed later.

[xeioex]

@hongzhidao

instanceof fix: https://gist.github.com/14331cb5d4df17d87d5dc210e56ec43e

[hongzhidao]

@xeioex

instanceof fix: https://gist.github.com/14331cb5d4df17d87d5dc210e56ec43e

NJS_APPLIED is completely removed.
https://gist.github.com/hongzhidao/5e966d8eed60f567e56f3f3ba2e12d9f

It seems all the tests passed.

[xeioex]

@hongzhidao

njs_function_call() and njs_function_apply()

They names should be swapped
Compare https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call

The call() method calls a function with a given this value and arguments provided individually.

with

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply

The apply() method calls a function with a given this value, and arguments provided as an array (or an array-like object).

[hongzhidao]

@xeioex

updated.
https://gist.github.com/hongzhidao/bfbf66f377b270bed5f79031cc829949
njs_function_call() and njs_function_apply() are swapped.

Split the big patch into small patches.
not sure it worth the efforts.

Agree, won't do it.

I'm on njs_function_native_call().

[xeioex]

@hongzhidao

The serious issue with different platforms (most notably 32bit platforms):
frame->retval is njs_index_t not njs_value_t.

We can ONLY cast (njs_value_t * value) to njs_index_t if value 16-byte aligned.
because (for example in njs_function_native_call())
njs_vmcode_operand(vm, frame->retval) == frame->retval only if
njs_scope_type(frame->retval) == 0 // NJS_SCOPE_ABSOLUTE
otherwise we get garbage result.

Consider njs_value_to_primitive()
we pass njs_value_t retval; // which is allocated on the stack

ret = njs_function_call(...(njs_index_t) &retval)

&retval is not necessary 16-byte aligned.

This can be fixed by using nxt_aligned(16) for each such declarations. But probably we should try to redo frame->retval update (from njs_index_t to njs_value_t *).

[xeioex]

@hongzhidao

First patch is updated: https://gist.github.com/ec83686ff442da2381aeaafa454232e3

Most notable change: https://gist.github.com/xeioex/ec83686ff442da2381aeaafa454232e3#file-github190-patch-L883

[hongzhidao]

@xeioex

Improved njs_function_native_call().
https://gist.github.com/hongzhidao/4f43555bb66073fceb0b50907b0b8cb7

njs_function_native_call() still need more time on reviewing.

BTW, many functions that return type is njs_ret_t can be rewritten with nxt_int_t.

[xeioex]

BTW, many functions that return type is njs_ret_t can be rewritten with nxt_int_t.

yes, It can be done later.

[hongzhidao]

yes, It can be done later.

I'll be on njs_function_native_call() tomorrow. It seems to that frame->skip is necessary.
Others are nearing completion. This is a good ticket.

[xeioex]

@hongzhidao

https://gist.github.com/hongzhidao/4f43555bb66073fceb0b50907b0b8cb7

 === Summary ===
  - Ran 27119 tests
- - Passed 8228 tests (30.3%)
- - Failed 18891 tests (69.7%)
+ - Passed 8187 tests (30.2%)
+ - Failed 18932 tests (69.8%)

Failed test example: https://gist.github.com/4bf26a7aa0bb71071889afad95738574

[hongzhidao]

Failed test example: https://gist.github.com/4bf26a7aa0bb71071889afad95738574

I think the bug happens in njs_function_native_call(). Need more time to make it robust.

[hongzhidao]

@xeioex

Failed test example: https://gist.github.com/4bf26a7aa0bb71071889afad95738574

Fixed.
https://gist.github.com/hongzhidao/97c86aedb904c140dbe5325549d4fecb

[hongzhidao]

@xeioex

Here's the final patch including your first updated patch.
https://gist.github.com/hongzhidao/4fbb5e719ca77c28e8757433460ab252

BTW, what about removing njs_vm_invoke()?

grep -r  njs_vm_invoke njs/
njs/njs.c:    return njs_vm_invoke(vm, function, args, nargs, NJS_INDEX_GLOBAL_RETVAL);
njs/njs.c:njs_vm_invoke(njs_vm_t *vm, njs_function_t *function, const njs_value_t *args,
njs/njs.h:NXT_EXPORT nxt_int_t njs_vm_invoke(njs_vm_t *vm, njs_function_t *function,
njs/njs_module.c:            ret = njs_vm_invoke(vm, &module->function, NULL, 0, module->index);

[xeioex]

@hongzhidao

Here's the final patch including your first updated patch.
https://gist.github.com/hongzhidao/4fbb5e719ca77c28e8757433460ab252

Great job! test262 passes, I am on review and plan to commit with some minor changes when all additional tests pass.

[hongzhidao]

@xeioex

Can NJS_MAX_STACK_SIZE be removed?

[hongzhidao]

@xeioex take a look, please.

Here are patches based on the latest released version.
https://gist.github.com/hongzhidao/ff73f8161f1e7e6929408e5322492f8f

1. Moving vm codes to njs_vmcode.c.
2. Moving value functions to njs_value.c
3. Moving vm functions to njs_vm.c and rename njs_shell.c as njs.c
   (I referred to something about LUA. https://github.com/lua/lua)
4. Refactored working with non-primitive types. (Your first patch.)

These make the file structure clearer. If OK, I recommend doing this first.

BTW, if we can eliminate frame->call and skip, it'll be ideal. But I'm not sure, on it.

[hongzhidao]

@xeioex

https://gist.github.com/hongzhidao/4fbb5e719ca77c28e8757433460ab252#file-njs-190-patch-L257
Why is there such a change? (plus 1)

[hongzhidao]

@xeioex

Here are the patches based on the latest released version.
https://gist.github.com/hongzhidao/60ba71ff508deaaa0feca81a721eb211

1. Moving vm code methods to njs_vmcode.c.
2. Moving value methods in njs.c to njs_value.c.
3. Moving vm methods in njs.c to njs_vm.c.
4. Rename njs_shell.c as njs.c.
5. Refactored working with non-primitive types.
6. Refactored working with synchronous calling.

[xeioex]

@hongzhidao

Moving vm code methods to njs_vmcode.c.
Moving value methods in njs.c to njs_value.c.
Moving vm methods in njs.c to njs_vm.c.

Maybe later, after Refactoring patches are committed.

not sure vmcode vs vm functions are worth to split. moving vm methods from njs.c to njs_vm.c looks useful.

rename njs_shell.c as njs.c

Actually, I like njs_shell.c (it was @igorsysoev choice also) for CLI file.

https://gist.github.com/hongzhidao/4fbb5e719ca77c28e8757433460ab252#file-njs-190-patch-L257
Why is there such a change? (plus 1)

Initially I got segfaults because of frames overlapping on 32bits. It was a workaround, will be fixed, thanks.

[hongzhidao]

Maybe later, after Refactoring patches are committed.

OK.

not sure vmcode vs vm functions are worth to split.

I also agree vm codes belongs to vm. But vm codes is a very central part in the script engine.
In lua, it named lcode.c. But I prefer njs_vmcode than njs_code.
Imagine that we may add more vm code methods, I would like to change njs_vmcode.c and keep njs_vm.c (it includes njs_vm_{methods}) unchanged.

moving vm methods from njs.c to njs_vm.c looks useful.

:) The interesting thing is it will make njs.c empty.

Actually, I like njs_shell.c (it was @igorsysoev choice also) for CLI file.

I also agree that njs_shell.c looks good. But consider it again.

[xeioex]

@hongzhidao

I see njs.c as a place for top-level functions (declared in public header njs.h) for njs users (as a library).
njs_shell.c and njs_unit_test.c are users for example.

[hongzhidao]

I see njs.c as a place for top-level functions (declared in public header njs.h) for njs users (as a library).
njs_shell.c and njs_unit_test.c are users for example.

Make sense. I agree we keep njs_shell.c.

moving vm methods from njs.c to njs_vm.c looks useful.

If njs.c is empty, do we remove it? Since the top-level functions are OK in non njs.c files.
For example, do you think it's OK we do Moving value methods in njs.c to njs_value.c.?
What I mean is the functions with NXT_EXPORT can be put in many files, such as njs_vm.c.

[xeioex]

@hongzhidao

If njs.c is empty, do we remove it? Since the top-level functions are OK in non njs.c files.

I would leave functions like njs_vm_compile, njs_vm_create() etc in njs.c

For example, do you think it's OK we do Moving value methods in njs.c to njs_value.c.?

it is OK.

[hongzhidao]

I would leave functions like njs_vm_compile, njs_vm_create() etc in njs.c

I know what you mean, get it. Do it as your way.

[xeioex]

@hongzhidao

Maybe you are right regarding njs.c njs_vm.c and njs_vmcode.c split, I will look at it later.

[hongzhidao]

I would leave functions like njs_vm_compile, njs_vm_create() etc in njs.c
Maybe you are right regarding njs.c njs_vm.c and njs_vmcode.c split, I will look at it later.

I thought it much more. I think njs_vm_create etc belongs to njs_vm.c. And refer to lua.
https://github.com/lua/lua/blob/master/lstate.c (state in lua is vm in njs)