Support recursive calls that respect merged functions

[nickewing]

This PR implements a method of recursion that works with merged typed-functions. The issue was originally discussed here: josdejong/mathjs#1885.

I tested this change against math.js, replacing all recursive typed-function calls I could find in the library, and the tests are passing. I can open a PR for that too, if this looks acceptable.

A slight modification on this approach would be to remove the proxy object and change

return fn.apply(proxy, arguments);

to

return fn.apply(self, arguments);

which would then allow definitions like so:

var sqrt = typed({
  'number': function (value) {
    return Math.sqrt(value);
  },
  'string': function (value) {
    return this(parseInt(value, 10));
  }
});

However, I am not sure if using a function bound as this works in all browsers.

[josdejong]

This is a really nice solution Nick 😎

It's a very smart idea to utilize the function's this, which we can determine ourselves when calling fn.apply. I'm not sure either if this will give issues in certain browsers, but I don't expect that. We should definitely give that a try. It would be a great fit since it works very natural and doesn't need a new API at all :).

If I'm not mistaken, when using the function's this (without the proxy and baseFn), it will be as simple as the following, right?

function compileArgsPreprocessing(params, fn, resolveSelf) {
  // ...
  return fn.apply(resolveSelf(), args);
  // ...
}

We should also double check if this has any impact on the performance.

[josdejong]

Maybe good to also demo console.log(sqrt('9')); here, passing a string as input 😄

[nickewing]

Haha, oops! Good catch.

[nickewing]

Thanks for the feedback. I think it's a good call to give directly binding the function a try.

Given the way compileArgsPreprocessing conditionally wraps the fn function, we can't completely remove the wrapper function (previously baseFn).

I refactored compileArgsPreprocessing since there was already a note in there that it could use some cleanup. If you think the refactor is too much, I can revert that commit.

[josdejong]

That is a really neat refactoring, thanks 👍

I tested in mathjs and it indeed just works replacing self references with this. I did run all tests with this on all browsers including IE, and it works like a charm.

I did run the benchmark in benchmark/benchmark.js, and this shows a serious decrease in performance. I think that needs some attention:

before recursive support:

> node benchmark/benchmark.js
typed add            x 32,011,095 ops/sec ±0.42% (89 runs sampled)
native add           x 34,924,831 ops/sec ±0.32% (94 runs sampled)

after recursive support:

> node benchmark/benchmark.js
typed add            x 23,669,110 ops/sec ±0.71% (95 runs sampled)
native add           x 34,913,476 ops/sec ±0.48% (95 runs sampled)

Can this be related to the extra indirection introduce with applyWithSelf? It may be worth trying to revert the refactorings, start from there adding resolveSelf() to see if this is a consequence of resolveSelf() or of the refactoring. And we should also be careful that maybe this micro benchmark can be off too. What do you think?

[nickewing]

Good call on checking the benchmarks. It seems that it wasn't actually the refactored compileArgsPreprocessing function that caused the slowness because after reverting that, I was still getting benchmark numbers similar to what you saw. It must have been something to do with using closure in the resolveSelf that was passed into compileArgsPreprocessing.

I reworked things again and now the numbers look to be comparable. There are couple slower runs in this branch and in the develop branch, but given that the native runs were also slow there, I'm guessing those aren't relevant. I did leave out the other function refactoring though given the changes to call resolveSelf were no longer necessary.

I also ran the new implementation against math.js, with all the recursive function calls replaced with this, and things looked ok there too.

recursion branch	develop branch
typed add x 31,814,191 ops/sec ±0.23% (97 runs sampled)	typed add x 30,844,791 ops/sec ±2.01% (90 runs sampled)
native add x 36,088,603 ops/sec ±0.51% (94 runs sampled)	native add x 36,104,421 ops/sec ±0.23% (99 runs sampled)
typed add x 23,485,833 ops/sec ±0.24% (96 runs sampled)	typed add x 30,438,337 ops/sec ±0.28% (87 runs sampled)
native add x 25,594,284 ops/sec ±0.28% (92 runs sampled)	native add x 35,077,959 ops/sec ±0.25% (99 runs sampled)
typed add x 31,598,207 ops/sec ±0.23% (94 runs sampled)	typed add x 30,879,719 ops/sec ±0.24% (92 runs sampled)
native add x 35,506,001 ops/sec ±0.21% (94 runs sampled)	native add x 35,375,359 ops/sec ±0.22% (97 runs sampled)
typed add x 30,558,612 ops/sec ±0.28% (91 runs sampled)	typed add x 31,482,917 ops/sec ±0.22% (93 runs sampled)
native add x 34,990,322 ops/sec ±0.32% (91 runs sampled)	native add x 35,711,849 ops/sec ±0.18% (98 runs sampled)
typed add x 25,565,964 ops/sec ±1.51% (96 runs sampled)	typed add x 31,196,506 ops/sec ±0.31% (92 runs sampled)
native add x 28,363,794 ops/sec ±0.10% (96 runs sampled)	native add x 36,093,807 ops/sec ±0.17% (96 runs sampled)
typed add x 31,570,449 ops/sec ±0.25% (94 runs sampled)	typed add x 25,673,182 ops/sec ±0.28% (93 runs sampled)
native add x 35,816,558 ops/sec ±1.36% (97 runs sampled)	native add x 28,387,732 ops/sec ±0.14% (96 runs sampled)
typed add x 31,125,150 ops/sec ±0.43% (94 runs sampled)	typed add x 30,437,918 ops/sec ±0.28% (96 runs sampled)
native add x 35,639,589 ops/sec ±0.17% (97 runs sampled)	native add x 34,852,741 ops/sec ±0.21% (94 runs sampled)
typed add x 31,335,404 ops/sec ±0.51% (92 runs sampled)	typed add x 31,240,131 ops/sec ±0.44% (91 runs sampled)
native add x 35,098,381 ops/sec ±0.20% (95 runs sampled)	native add x 36,110,116 ops/sec ±0.15% (98 runs sampled)
typed add x 31,178,082 ops/sec ±0.30% (94 runs sampled)	typed add x 31,251,682 ops/sec ±0.37% (92 runs sampled)
native add x 35,338,348 ops/sec ±0.16% (94 runs sampled)	native add x 36,057,924 ops/sec ±0.25% (95 runs sampled)
typed add x 25,724,821 ops/sec ±0.28% (94 runs sampled)	typed add x 31,288,245 ops/sec ±0.25% (90 runs sampled)
native add x 28,578,597 ops/sec ±0.15% (97 runs sampled)	native add x 36,064,112 ops/sec ±0.20% (94 runs sampled)

[josdejong]

That's awesome news 🎉 ! Your latest PR is very nice and simple. I've done some testing and can confirm that there is no decrease in performance at all.

Maybe one last small simplification step: the following chunk of code:

      // create the typed function
      // fast, specialized version. Falls back to the slower, generic one if needed
      var makeFn = function () {
        var fn = function fn(arg0, arg1) {
          'use strict';

          if (arguments.length === len0 && test00(arg0) && test01(arg1)) { return fn0.apply(fn, arguments); }
          if (arguments.length === len1 && test10(arg0) && test11(arg1)) { return fn1.apply(fn, arguments); }
          if (arguments.length === len2 && test20(arg0) && test21(arg1)) { return fn2.apply(fn, arguments); }
          if (arguments.length === len3 && test30(arg0) && test31(arg1)) { return fn3.apply(fn, arguments); }
          if (arguments.length === len4 && test40(arg0) && test41(arg1)) { return fn4.apply(fn, arguments); }
          if (arguments.length === len5 && test50(arg0) && test51(arg1)) { return fn5.apply(fn, arguments); }

          return generic.apply(fn, arguments);
        }

        return fn;
      }

      var fn = makeFn();

can be simplified to:

      function fn (arg0, arg1) {
        'use strict';

        if (arguments.length === len0 && test00(arg0) && test01(arg1)) { return fn0.apply(fn, arguments); }
        if (arguments.length === len1 && test10(arg0) && test11(arg1)) { return fn1.apply(fn, arguments); }
        if (arguments.length === len2 && test20(arg0) && test21(arg1)) { return fn2.apply(fn, arguments); }
        if (arguments.length === len3 && test30(arg0) && test31(arg1)) { return fn3.apply(fn, arguments); }
        if (arguments.length === len4 && test40(arg0) && test41(arg1)) { return fn4.apply(fn, arguments); }
        if (arguments.length === len5 && test50(arg0) && test51(arg1)) { return fn5.apply(fn, arguments); }

        return generic.apply(fn, arguments);
      }

And lastly, we should also document this new feature in the API description in the README.md. If you want I can do that, no problem.

[nickewing]

That really cleans it up nicely! That would be great if you added the details about this in the README. I wasn't sure which sections this feature should be mentioned in.

Once this is merged and a new release is published, I can open a PR in math.js to replace all the existing self references.

[josdejong]

Thanks Nick, looks great 😎 . I've merged your PR and published typed-function@2.0.0. (published as v2.0.0 and not v1.2.0 since the library officially drops support for node.js 6 and 8).

Thanks for also working on the changes in mathjs, if you need help there please let me know, it may be quite some work.

[nickewing]

That's great - thanks @josdejong!

I had already replaced all the instances of recursion I could find in math.js to test against this new functionality locally. I've opened a PR here: josdejong/mathjs#1903

[josdejong]

👍