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

Added generateArgs() function #2308

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

denis-migdal
Copy link
Contributor

New method generateArgs(fct, name, args_str, defaults) :

generateArgs( {}, "__init__", "self, *args", {self: null}); // last argument is the default values.

Please do not merge yet, I'm working on addMethod() now.
It is just for you to see generateArgs().

@denis-migdal
Copy link
Contributor Author

New method addMethod(kclass, fct, args_str, defaults):

addMethod( list, __init__, "self, *args", {self: null}); // last argument is the default values.

@denis-migdal
Copy link
Contributor Author

denis-migdal commented Nov 12, 2023

Got an idea for better perfs.

Instead of having the internal function like : foo({argsname1: value1, argsname2: value2}).
We could have the internal functions like : foo(pos_argname, pos_argname, vargargs_argname, kw_argname, kw_argname, kwargs_argname).

This would prevent having to build an object when calling the function internally.
But this would require to create a new genArgs0 function for internal functions, in order to return an array instead of an object. Though it might potentially enable quicker argument parsing when calling it.

@PierreQuentel should I try to do it ?

@denis-migdal
Copy link
Contributor Author

@PierreQuentel Tell me which one do you prefer, and I'll make a cleaner commit with parsing method generation (like we did for $B.args0()).

Solution 1 : more optimized

function foo(self, args_a, args_b,
                            varargs, // JS array
                            kwargs // JS object
                            ){
    // do stuff
}

addMethod(klass, foo, "self, args_a, args_b, *args, **kwargs");

foo(self, 1, 2, [3,4], {a: 34});

Solution 2 : less optimized, use $B.args0_NEW

function foo({self, args_a, args_b,
                            varargs, // PY tuple
                            kwargs  // PY dict
                            }){
    // do stuff
}

addMethod(klass, foo, "self, args_a, args_b, *args, **kwargs");

foo({self, args_a: 1, args_b: 2,
            varargs: $B.fast_tuple([3,4]),
            kwargs: $B.obj_dict({a: 34})
        });

@PierreQuentel
Copy link
Contributor

I understand the options for function foo(...) and addMethod, but what is the next line foo(...), is it how the function is supposed to be called ?

Calls are constructed in ast_to_js.js / $B.ast.Call.to_js(), which itself calls make_args(). I am not opposed to changing the implementation if it results in an overall faster execution, but it also means that internal calls (inside Brython core scripts) to internal functions will have to be adapted.

@denis-migdal
Copy link
Contributor Author

I understand the options for function foo(...) and addMethod, but what is the next line foo(...), is it how the function is supposed to be called ?

For the internal function, yes. Like you already have both an internal and external functions for some methods.

Internal functions doesn't do arguments parsing, and is like a shortcut when you use it internally. Whereas external calls do the argument parsing (so is a little slower).

addMethod(klass, ...) adds a method to the class klass, that is called like a Brython method is called :

foo(self, 1, 2, {}) // internal call (e.g. called from JS files like py_lists.js).
self.klass.foo(self, 1, 2, {$kw: [{}, {}]}) // external call (e.g. called from a Brython script).

// external calls is implemented like :
self.klass.foo = function(...args) { return foo(...parseArgs(foo, args) )  }

Calls are constructed in ast_to_js.js / $B.ast.Call.to_js(), which itself calls make_args().

Interesting. Later, I'll have some optimizations to suggest in regard of it.
Lot of optimizations can be made, but for now I'll focus on the most expensive stuffs.

Don't hesitate to give me some codes you want to benchmark (only using core features, without prints, DOM manipulations, or python libraries).

but it also means that internal calls (inside Brython core scripts) to internal functions will have to be adapted.

Yep, but you can adapt it little by little, changing one method at a time.
You can find all the internal calls of a method, by using tools like grep.

The advantage, is that it should be very fast during execution time.
Currently, external calls use the old parsing algorithm. The new implementation should be even faster than current parsing :

  • no needs to convert kwargs and varargs to dict or tuple (because internal use).
  • uses an array instead of an object (because no needs of locals internally).

@denis-migdal
Copy link
Contributor Author

denis-migdal commented Nov 19, 2023

Did some benchmark (foo(i) / N=100000000), here the conclusions :

  1. Internal foo(i) calls is 2.5x faster than foo({i}) call.
  2. Parsing ($B.args0) + internal foo(i) call is 27.6x slower than internal call.
  3. Current implementation ($B.args) is 2x slower compared to using $B.args0.
  4. Using a parsing function that returns an object, extracting the arguments, then calling a function, has a ~1.5% overcost (so not a lot).
  5. New suggested method is slower than using current implementation of $B.args0, but I noticed I didn't write it from the last optimized version of $B.args0, so I'll have to redo the benchmark.

EDIT: This means that if you replace some current calls to internal calls, you might be at least 50x faster.

@denis-migdal
Copy link
Contributor Author

Okay, did better benchmark $B.args0 vs new parser :

  • with 1 argument : no big differences
  • with 3 arguments : 10-15% faster
  • with *vargargs : 2x faster
  • with **kwargs : 6x faster ???

I cheated a little (shame on me) for some stuff I know will be solved by a parser generator.

I didn't tested named argument but it should be faster :

  • string keys are replaced by integer keys (faster)
  • for named arguments, this requires to convert the name into an index using Map.get(), but allows me to not having to use argnames[i] when verifying the final result, which is faster.
  • when having a **kargs parameter, I need to use Map.get instead of Map.has, which funny enough is faster.

So overall it shouldn't be slower, and should even be faster.

@PierreQuentel I assume this is worthing it ?

I also noticed that I missed some micro optimizations in $B.args0_NEW...

@denis-migdal

This comment was marked as outdated.

@denis-migdal
Copy link
Contributor Author

Okay, so after further tests :

  1. Using eval() or Function() is really really slow, better avoiding it. Function() has the same execution speed than normal functions, but build time is very very slow.
  2. Automatically converting the result of $B.args0 into an array costs +26% in execution time.
  • so either use the new parsing function.
  • or make the internal function taking an object as parameter.
  1. Having an internal and external function, with the external function calling the internal after parsing the arguments, seems to have no over cost compared to just having the external function. Browser seems to inline the function ?

For 2., indeed, we can't do :

klass.fooE = function() {
    const result = $B.args0(__args__, arguments);
    return fooE( result.i );
}

automatically, so we must do something like :

const argnames = ["i"];

klass.fooC = function() {
    const result = $B.args0(__args__, arguments);
    const args = new Array(argnames.length);
    for(let i = 0; i < argnames.length; ++i)
        args[i] = result[argnames[i]];

    return fooC( ...args );
}

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.

2 participants