refactor(Subscription): remove to use tryCatch

[tetsuharuohzeki]

• This is just refactoring to remove to use tryCatch.
• To make types more strictly, we would need to overhaul Subscription and separate an interface from Subscription class...

[trxcllnt]

@saneyuki have you compared the performance between master and this branch? I'd be interested to see what sort of difference this makes here.

[tetsuharuohzeki]

@trxcllnt

I added benchmarks and the following are results (Node 5.7, MacBook Pro (Retina, 15-inch, Mid 2014, 2.8 GHz Intel Core i7, 16 GB 1600 MHz DDR3)):

Before

                                         |                     RxJS 4.0.7 |              RxJS 5.0.0-beta.2 |          factor |      % improved
---------------------------------------------------------------------------------------------------------------------------------------------------
                            add-function |             1,754,485 (±0.60%) |             8,995,730 (±0.50%) |           5.13x |          412.7%
                        add-subscription |             1,735,979 (±0.72%) |             8,677,772 (±0.63%) |           5.00x |          399.9%
                     remove-subscription |               917,388 (±0.70%) |             1,738,510 (±0.63%) |           1.90x |           89.5%
    unsubscribe-simple-with-unsubscriber |            44,508,246 (±0.69%) |            10,933,015 (±0.70%) |           0.25x |          -75.4%
 unsubscribe-simple-without-unsubscriber |            47,338,542 (±1.01%) |            43,357,329 (±0.57%) |           0.92x |           -8.4%
               unsubscribe-with-children |             1,447,677 (±0.72%) |             1,427,434 (±0.44%) |           0.99x |           -1.4%
             unsubscribe-with-descendant |             2,012,364 (±0.87%) |             2,417,973 (±0.68%) |           1.20x |           20.2%

After to remove tryCatch

                                         |                     RxJS 4.0.7 |              RxJS 5.0.0-beta.2 |          factor |      % improved
---------------------------------------------------------------------------------------------------------------------------------------------------
                            add-function |             1,740,157 (±0.59%) |             8,609,207 (±0.50%) |           4.95x |          394.7%
                        add-subscription |             1,713,813 (±0.59%) |             8,671,869 (±0.57%) |           5.06x |          406.0%
                     remove-subscription |               920,657 (±0.58%) |             1,742,495 (±0.47%) |           1.89x |           89.3%
    unsubscribe-simple-with-unsubscriber |            42,486,389 (±0.78%) |            17,084,885 (±0.77%) |           0.40x |          -59.8%
 unsubscribe-simple-without-unsubscriber |            45,266,060 (±0.78%) |            39,835,860 (±0.47%) |           0.88x |          -12.0%
               unsubscribe-with-children |             1,424,311 (±0.70%) |             2,627,288 (±0.89%) |           1.84x |           84.5%
             unsubscribe-with-descendant |             2,002,804 (±0.67%) |             3,592,306 (±0.58%) |           1.79x |           79.4%

[benlesh]

These external functions are going to introduce closure when they're used. If they were on the Subscription prototype, they would not.

[tetsuharuohzeki]

These external functions are going to introduce closure when they're used

This callPrivateUnsubscribe don't capture a variables in outer environments. This would not be a closure which capture an environment. What would be a problem?

[tetsuharuohzeki]

@Blesh

---

A faster approach is to call a function that only has the try/catch block in it, that in turn calls another method that doesn't have any try/catch blocks in it (and optimizes better).
#1389 (comment)

I tried add try***Unsubscribe() methods to Subscription class, however, if we will do it, we cannot make them private methods because Subject<T> implements Subscription and Subject<T>.unsubscribe() calls Subscription.prototype.remove.call(this, subscription).

To add try***Unsubscribe() as private methods might be able to do it if we reorder their interfaces and class, but I think we should not do it in this pull request because it may be a large and complex.

So I propose that we concentrate to "remove tryCatch from Subscription" on this pull request.

How about do you think?

[coveralls]

Coverage decreased (-0.0006%) to 95.98% when pulling 53630c6 on saneyuki:subscription into 34a0d3c on ReactiveX:master.

[benlesh]

If you're asserting "truthiness" here, you can just do it, no need for the !!. If you truly need a boolean, then Boolean(x) is probably more efficient than two operators back to back (but I'm speculating)

[benlesh]

Left a few more comments. This needs rebased again. Sorry for the slowness with the merge, but Subscription is one area we have to get right.

[benlesh]

This seems to have died out. I'm going to close it for now, because I think that the code has diverged significantly since this PR was made.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.