Njs: function parameter issue

[hongzhidao]

function test(option) {
    option = {
        max: option
    }

    console.log(option);
}

test(3);

Output in njs

{max:[Circular]}

Output in node

{ max: 3 }

It works well outside functions.

var option = 3;
option = {
  max: option
};
console.log(option);

[xeioex]

Related to https://github.com/nginx/njs/issues/267 and https://github.com/nginx/njs/issues/270

[hongzhidao]

@xeioex
Is there anyone on it?
If not, I plan to fix it.

[xeioex]

@hongzhidao

Nobody was assigned to it. I may require changes to parser/generator. If it is true, I would not touch it until refactoring by @lexborisov is done because merge will be hard.

You may want to look into https://github.com/nginx/njs/issues/271, it looks like it does not require much changes.

[hongzhidao]

@xeioex
Do you suggest fixing it until the refactoring of parser/generator is done by @lexborisov ?

[xeioex]

@hongzhidao

yes.

[hongzhidao]

@lexborisov

I would not touch it until refactoring by @lexborisov is done because merge will be hard.

I noticed that you use rbtree instead of lvlhsh in some places. Can you share why?
I don't know how to decide which one to use if I need to use hash-like functions.

[lexborisov]

Hi @hongzhidao

Yes, after refactoring lexer in the njs, three lvlhsh were replaced to rbtree.
Currently, a single hash table is used for all variable names. Please, see one and two.

That is, when lexer sees the name of the variable, he tries to match it in the hash-table.
The found entry in the table is assigned to the token - this is a unique identifier.

Further, we save unique identifiers to a scope (for AST node). Just for this, trees are well suited.

Now, to check the existence of a variable in the scope, it is enough to compare the numbers in the rbtree. Before that, I had to look in the hash table and compare the names of variables. For many functions, we had to pass a hash identifier and a string with the name of the variable for matching. It was very wasteful in all plans. Now only a unique identifier need to pass in functions.

Simply put, hash tables are better for matching names. To store and search for numbers use trees.
But, everything is as usual in life, a little more complicated :)
Read how indexes are arranged in popular databases for a better understanding of the use of trees.

For example, suffix trees are used to quickly search for text.
In general, in each case you have to choose the most suitable solution.

[hongzhidao]

@lexborisov

1. it's a clever improvement, what's your plan of the rest in refactoring lexer?

2. patch with simplifying njs_lexer_keyword() function.
   Just a little rework, take a look, please.

diff --git a/src/njs_lexer_keyword.c b/src/njs_lexer_keyword.c
index 310e999..ab46787 100644
--- a/src/njs_lexer_keyword.c
+++ b/src/njs_lexer_keyword.c
@@ -15,12 +15,12 @@ njs_lexer_keyword_hash(const u_char *key, size_t size, size_t table_size)
 }


-njs_inline const njs_lexer_keyword_entry_t *
-njs_lexer_keyword_entry(const njs_lexer_keyword_entry_t *root,
-    const u_char *key, size_t length)
+const njs_lexer_keyword_entry_t *
+njs_lexer_keyword(const u_char *key, size_t length)
 {
-    const njs_lexer_keyword_entry_t  *entry;
+    const njs_lexer_keyword_entry_t  *root, *entry;

+    root = njs_lexer_keyword_entries;
     entry = root + njs_lexer_keyword_hash(key, length, root->length);

     while (entry->key != NULL) {
@@ -29,31 +29,14 @@ njs_lexer_keyword_entry(const njs_lexer_keyword_entry_t *root,
                 return entry;
             }

-            entry = &root[entry->next];
-
         } else if (entry->length > length) {
             return NULL;
-
-        } else {
-            entry = &root[entry->next];
         }
-    }
-
-    return NULL;
-}
-

-const njs_lexer_keyword_entry_t *
-njs_lexer_keyword(const u_char *key, size_t length)
-{
-    const njs_lexer_keyword_entry_t  *entry;
-
-    entry = njs_lexer_keyword_entry(njs_lexer_keyword_entries, key, length);
-    if (njs_slow_path(entry == NULL)) {
-        return NULL;
+        entry = root + entry->next;
     }

-    return entry;
+    return NULL;
 }

1. I recently did a work of http route, it's like the location matching in nginx.
   I choose radix-tree. https://gist.github.com/hongzhidao/91aad419800d9e5331bd83cacbefadb4
   Do you think it's a good alternative if we redesign how to implement nginx static locations matching? (not really do it in nginx. actually, I want to apply in unit if it's welcome :)

[lexborisov]

@hongzhidao

@lexborisov

1. it's a clever improvement, what's your plan of the rest in refactoring lexer?

With the lexer finished. In the future, some functions will be removed from there.
Now I'm refactoring the parser.

1. patch with simplifying njs_lexer_keyword() function.
   Just a little rework, take a look, please.

Thanks, I liked that you saw how you can optimize the search on the table, got rid of the "else".
But combining functions is not worth it. This is a logical separation.

At the same time, the njs_lexer_keyword function looks really strange.
I would reduce it to such a state:

const njs_lexer_keyword_entry_t *
njs_lexer_keyword(const u_char *key, size_t length)
{
    return njs_lexer_keyword_entry(njs_lexer_keyword_entries, key, length);
}

1. I recently did a work of http route, it's like the location matching in nginx.
   I choose radix-tree. https://gist.github.com/hongzhidao/91aad419800d9e5331bd83cacbefadb4
   Do you think it's a good alternative if we redesign how to implement nginx static locations matching? (not really do it in nginx. actually, I want to apply in unit if it's welcome :)

I will definitely look at it in detail today and say my opinion.

[lexborisov]

@hongzhidao Let's get opinions from Maxim ( @mar0x principal programmer on Unit).

I recently did a work of http route, it's like the location matching in nginx.
I choose radix-tree. https://gist.github.com/hongzhidao/91aad419800d9e5331bd83cacbefadb4
Do you think it's a good alternative if we redesign how to implement nginx static locations matching? (not really do it in nginx. actually, I want to apply in unit if it's welcome :)

It seems to me that radix-trees here will not be entirely appropriate. But Maxim knows Unit better and will tell us something.

[hongzhidao]

@lexborisov
Take a look again.
https://gist.github.com/hongzhidao/e9dd94f50fbc44a222dd85c38fef8b26

Before that, it seems it's better to put njs_lexer_keyword_find into njs_lexer_keywords.c as the prefix njs_lexer_keyword_. In njs_lexer_word, we can call all the tokens consisted of [a-zA-Z] as word, entry. There are two types of them, keyword and name, corresponds to NJS_TOKEN_{KW} and NJS_TOKEN_NAME.
So I think it would be better to get understand by renaming njs_lexer_keyword_find as njs_lexer_name_find, rename keywords_hash as names_hash.

I don't know what name will better when I want to express the logic includes find or insert,
related to UNIT, may it's better to use njs_lexer_name_get since find has no side-effect.
https://github.com/nginx/unit/blob/master/src/nxt_runtime.c#L1396

But combining functions is not worth it. This is a logical separation.

njs_lexer_keyword() just has one line code, can you share why not combine them?

BTW, what about removing all from njs_lexer_keyword.c into njs_lexer.c?
Now, the file njs_lexer_keyword.c is too small.

[lexborisov]

@hongzhidao

Maybe you're right, yes.
keywords_hash => names.
njs_lexer_keyword_find => njs_lexer_name_find

njs_lexer_keyword() just has one line code, can you share why not combine them?

Above, I answered why.

BTW, what about removing all from njs_lexer_keyword.c into njs_lexer.c?
Now, the file njs_lexer_keyword.c is too small.

Separation of logic. I am an opponent to one file with different functionality.

[lexborisov]

@hongzhidao

Lexer and parser refactoring done in 86f55a7dc4a4.

[hongzhidao]

@xeioex @lexborisov
I think unique identifier can be more generic. All the values keyword, variable, property, etc can be converted to be unique identifier first, then all the places related to them always use unique identifier. Below I like to call unique identifier as entry.

See the example var a = b;. Now, variable b is converted to entry in the parsing phase. In the runtime phase, variable name b is processed again for prop_get.
I have a big impression of the JSAtom in quickjs. It makes the engine simple.
Another thing is JSShape, but I think JSAtom is the most important tricky.

I suggest introducing njs_entry_t and njs_property_t.
njs_entry.h/c: do the things related to unique identifier, rbtree will be required.
njs_property.h/c: it seems it's better to rename njs_object_prop as njs_property, including all the
things related to properties.

It would be welcome to have a API like this.

int JS_DefineProperty(JSContext *ctx, JSValueConst this_obj,
                      JSAtom prop, JSValueConst val,
                      JSValueConst getter, JSValueConst setter, int flags)

BTW, It seems the work of parsing is easy from now on benefit from the great work of phasing refactoring by @lexborisov.

[xeioex]

@hongzhidao

but I think JSAtom is the most important tricky.

Agree. We plan to refactor strings using this idea, in the mid-term future.

[xeioex]

@hongzhidao

BTW, AST, can be debugged now as follows:

echo "(1*(2+3+1/12))" | ./build/njs -a - | ruby -ryaml -rjson -e 'puts YAML.dump(JSON.parse(STDIN.read))'
---
name: END
right:
  name: MULTIPLICATION
  left:
    name: NUMBER
    index: 1757C90
    value: 1
  right:
    name: ADDITION
    left:
      name: ADDITION
      left:
        name: NUMBER
        index: 1757CA0
        value: 2
      right:
        name: NUMBER
        index: 1757CB0
        value: 3
    right:
      name: DIVISION
      left:
        name: NUMBER
        index: 1757C90
        value: 1
      right:
        name: NUMBER
        index: 1757CC0
        value: 12

[maxfortun]

Another example of this behavior with arrays:
test.js

function test(args) {
        if(!Array.isArray(args)) {
                args = [ args ];
        }
        console.log(args);
}

test("string");
test([ "array" ]);

Expected:

bash-4.2# node test.js
[ 'string' ]
[ 'array' ]

Result:

bash-4.2# njs test.js 
[[Circular]]
['array']

[lexborisov]

Fixed in a2c357b.