add the spread operator: function calls #78
Comments
|
@xeioex As everything needed to make it work already is implemented, I managed to come up with a patch for njs_parser.c: https://gist.github.com/277hz/b68936f0b845d96a3dda2bfaf025cb69 Edit: For arrays it is also already implemented halfway: Edit2: Sorry, found a bug, will rework it. Applying the same logic to array does however seem to work, too. |
|
Hi @277hz , Thanks for the patch Before submitting next time, please consider running |
|
I spent some more time trying to solve this and think I am very close now to a working solution for functions and arrays. Edit: Getting the same results in e.g. Chrome: Edit2: test262 is running... Edit3: Edit4: Failing happens for iterable objects as far as I can tell. |
|
Not all aspects of spreading will be solveable in the parser, but it should reduce the complexity without being much of a performance penalty. |
|
Getting closer :) |
|
@drsm @xeioex @lexborisov My idea is to add an operation if a name to be spread is found. Then copy the spread variable into the target array during, or more precise just before, NJS_VMCODE_PROPERTY_INIT and shift the following indexes accordingly. For this I would like to overwrite property of njs_vmcode_prop_set_t. Is there any way to make sure the link is not used anywhere else, or would it be better to just alloc a new njs_value_t and set the property operand to that link? |
I don't think this is possible, because the shift index is dynamic. Maybe the same way we do in template literals will work: >> var a = [1,2,3]
undefined
>> var t0 = Array.prototype.concat.call([], 4, a)
undefined
>> t0[Symbol.isConcatSpreadable] = false
false
>> var b = Array.prototype.concat.call([], 0, a, t0)
undefined
>> b
[
0,
1,
2,
3,
[
4,
1,
2,
3
]
] |
Using concat was on my mind, too, but I am looking for a way to avoid the overhead of allocating and copying over and over. Edit: Same with node: Edit2: |
|
Small comparison between output of node and my evolving patch, first line corresponds to node, second to njs: Did I miss any other weird combinations that are possible in javascript? Performance wise it looks like this: |
How about > var x = []; x[Symbol.iterator] = () => [1,2,3].values(); [...x]
[ 1, 2, 3 ]Also, just to be sure: > var x = [,]; Object.keys([...x,,...x])
[ '0', '2' ]
> var x = [,]; [...x].every((x) => typeof x == 'undefined')
true |
|
Hi @277hz If your patch is ready I can review it on Monday. |
|
@drsm Thank you, just the examples I was looking for. @lexborisov: Whats mostly missing still, is support for spreading function arguments, as I have not decided on how to manipulate the list of arguments passed to the function yet. While I think I am getting closer, Monday will not be the finish line I suppose :). Edit: |
|
Almost there: |
|
@lexborisov https://gist.github.com/277hz/b68936f0b845d96a3dda2bfaf025cb69 I just ran a final test262 with no difference to my previous post and did not happen to find any non working circumstances whatsoever. Edit: |
>> var a = [], b = () => [...a,...a]; b();
Segmentation fault (core dumped)> [...([1,2,3])]
[ 1, 2, 3 ]
> [...(void 0, [1,2,3])]
[ 1, 2, 3 ]
> var a = [1,2,3]; [...(a)]
[ 1, 2, 3 ]
> [...{ [Symbol.iterator]: () => [1,2,3].values() }]
[ 1, 2, 3 ]
> [...{}]
Uncaught TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator)) |
|
@drsm Working on it and still confident that the idea of shifting the njs_vmcode_prop_set_t before adding them to the array is a good way to go. To me it looks like spreading of objects could be taken care of in a similar fashion. Edit: |
|
Hi @277hz Thanks for the work you've done. |
|
@lexborisov Not yet, still have some work to do. After fixing what @drsm described, I think there are some situations left that do not run smoothly. Edit: I guess it is time for garbage collecting 😉, do you have any plans for implementing? Edit2: |
|
@lexborisov Final patch https://gist.github.com/277hz/b68936f0b845d96a3dda2bfaf025cb69 Passing all spread tests in test262, except those using a generator function: Edit: Edit2: |
|
I looked at your patch, and before doing anything with spread, we need to implement iterators and generators. This is what I am busy with now. After that, I'll go back to your patch. |
|
@lexborisov Thank you for taking the time.
Additionaly I moved most of the spread logic into its own file, except for what is needed in the parser and generator. To make sure results are as expected, I conducted a little bash script for comparing output between njs and node, currently containing 115 expressions and 10 benchmarks, that you are free to look up here compare_njs_node.sh. If you come up with more ideas for what to test, just let me know. |
node uses V8, the state-of-the-art JIT compiler for JavaScript (developed for more than 10 year by hundreds of people), it is super fast but uses some low-level magic for each supported architecture (x86, ARM, MIPS), it is also very large (28 mb executable). njs is interpreter, it is better to compare it to other interpreters (ducktape, quickjs, xs etc.). But we plan to work on execution speed also:
|
|
@xeioex Don't get me wrong 😄. I really like how you designed njs, and of course nginx, too...
I stumbled upon that during testing of spreading objects, order can really be important when accessing properties. Other than that what really stood out to me is the execution time of for and while loops. |
./build/njs -d
interactive njs 0.6.1
v.<Tab> -> the properties and prototype methods of v.
>> for (let i = 0; i < 100; i++);
shell:main
1 | 00000 LET 0221
1 | 00016 MOVE 0221 0233
1 | 00040 JUMP 48
@ 1 | 00056 POST INC 0043 0221 0221
@ 1 | 00088 LESS 0043 0221 0433
@ 1 | 00120 JUMP IF TRUE 0043 -64
1 | 00144 STOP 0033
undefinedHere, basically we do the large switch over and over again for each instruction marked with '@', JIT compiler eliminates the switch, and execute a cycle with almost native speed. It also may have code-flow analyser and which may eliminate the loop completely as not having any side-effects. |
|
We greatly appreciate your effort and enthusiasm.
The best way to succeed is to start small, with small changes, gradually moving toward more advanced features (spanning many subsystems). Please also note that: njs is not an experimental code, it is used in production. That is why we are hesitant about quickly accepting large code chunks. |
|
@xeioex I am trying to obey. At first I thought it is going to be smaller changes only, the biggest part acutally consists of the routine for shifting array indexes. While spending more time on the topic, there was just no reason to not add support for objects and functions, too. Ending up in restructuring my code into a new file, making it a lot cleaner and easier to reverse if required, I avoided touching anything existing besides adding some code to the parser for putting the required token into place and generator for consuming it. While trying to mimic your style from what I see, it is always harder being outside of the team and guess what the preferred way is. One thing I can assure though is my commitment to writing code that works, commenting it extensively and testing thoroughly. In case you are curious, this is how native for and while perform: -O3 is surely outstanding, as all three yield the same result. |
|
Patch updated: njs_spread.diff Attached the latest feature.diff: Performance is like this: Edit: Made AST work, ast_disassembled.txt. |
|
While providing the latest update for njs_spread.diff and hoping to come nearer to an end 😄, I wanted to shed some insight on what the patch modifies technically. It would be nice if someone finds the time for reviewing, I greatly appreciate it. For "^" NJS_TOKEN_NAME_SPREAD with a reference to spread source (right) and spread target (left) is injected during parsing. For every injected NJS_TOKEN_NAME_SPREAD not an end indicator one njs_vmcode_name_spread_t with three operands (spread target type, spread target, spread source) is inserted after generating code for the spread source (right) first. Copying from spread target into spread source is handled in vm. When NJS_VMCODE_NAME_SPREAD is run vmcode is iterated until end indicator is found, setting the newly introduced shifted_property operand of related njs_vmcode_prop_set_t in between. Finally sum of sizeof one njs_vmcode_name_spread_t and one njs_vmcode_prop_set_t is returned as njs_jump_off_t. The shifted_property operand is then preferred during upcoming NJS_VMCODE_PROP_INIT. Resizing of spread targets and filling in invalid values if applicable is performed accordingly. This can also be visualized in AST and disassembled code neatly after applying the patch. While I see room for improvements, this approach has the advantage of NJS_VMCODE_NAME_SPREAD always being at the desired position within the target when run in vm and having the possibility to respect Symbol.iterator. Additionaly changes to the code base are minimized, except for adding one function to the generator and the modifications to the parser all other code is contained in its own source files. Moreover one might argue that it is performing fast enough already. The only case inoperable for now is copying to and from large not flat arrays. |
|
@277hz //Handled in parser without additional vmcode:
[ ...[1,2,3], ...[1,2,3], 4, 5, 6 ]
(()=>{})( ...[1,2,3], ...[1,2,3], 4, 5, 6 )there is a weird case that can't be handled in compile time: > var x = Array.prototype[Symbol.iterator]; Array.prototype[Symbol.iterator] = () => [1,2,3].values(); var y = [...[]]; Array.prototype[Symbol.iterator] = x; y;
[ 1, 2, 3 ] |
|
Thank you for your big work! Before starting a joint review of your patch, I suggest implementing generators (no asynchrony). Spread is uses with iterators and generators. |
|
Hi @277hz How are you? Are you still interested in this issue? |
http://www.ecma-international.org/ecma-262/6.0/#sec-argument-lists-runtime-semantics-argumentlistevaluation
http://www.ecma-international.org/ecma-262/9.0/#sec-argument-lists
The text was updated successfully, but these errors were encountered: