Rxjs: Speed refactor todos

[benlesh]

After talking with @briancavalier, I think there is a lot we can do to speed up RxJS even more.

Big takeaways from my talk with Brian:

1. Most.js's architecture isn't too different from RxJS, honestly.
2. Need to investigate our Subscriber structure. IMO, it's probably a mistake that we're inheriting from Subscriber everywhere. We should probably be doing something more basic. Subscriber has some guarantees built in, guarantees that aren't necessary anywhere but at the edges of the observable chain. We need to guarantee the user-provided observers, not the observer for the map operator.
3. Schedule everything might be a better strategy than we thought. This is because it enables us to move all error handling into the scheduler. Basically any error that occurs happens at some point in the stack _above_ the scheduled task, which is what has the try/catch in it. That error unwinds that, is caught, and then is sent down the error path in the observer chain.

This isn't something we'll try to squeeze into this release, rather, I want to document it, discuss it, and put it on the road map for refactors or possibly v6 (depending on what changes might need to be made).

[benlesh]

Also of note: I think the above described work would reduce the size of the library substantially as well.

[jadbox]

@blesh Thanks for taking the time to research performance. I've been impressed with @briancavalier ability to push JS performance beyond what I thought was possible.

possibly v6

I'd be willing to try to help as much as I can to get this into v5 once we have a plan. I'm on the verge of swapping out RxJS 5 on the server side for Most as the performance difference holds even in our real-world use-case.

Btw, I'm not sure how much setImmediate is used, but it's getting a large performance boost soon:
https://github.com/nodejs/node/pull/6436

[staltz]

:+1: this is the type of cross-pollination we need

[benlesh]

FWIW: I have a spike that implements what I've learned, but it's likely we couldn't do it until a v6... because there would be a few breaking changes. Also there are some questions about how this architecture will behave for retries and catches... I was talking with @jhusain and it seems this architecture, at least the error handling aspect of it, might fall on it's face with a mid-stream retry.

For example:

Observable.interval(100)
  .do(x => {
    if (x === 5) throw new Error('lol')
  })
  .retry(2)
  .flatMap(x => Observable.of(x).delay(200))
  .subscribe(x => console.log(x));

should show:

1
2
3
4
0
1
2
3
4

But with this architecture may show:

1
2
0
1
2

Because the error will unwind the stack all the way back to the producer, then propagate up and cause unsubscription of the inner subscriptions in the flatMap when it shouldn't.

This is just a suspicion, so I'll need to play with it. It's possible that the RetryObserver needs to be set up like a ProducerObserver and keep it's own guarantees, but I'm not sure.

[trxcllnt]

@blesh my gut tells me we'll get the most mileage from optimizing subscriber (even using a fast subscriber internally) than from scheduling everything.

You can't handle errors at the top level unless you keep a stack of observers to notify when errors propagate back up, and it's possible the extra bookkeeping would mitigate or reverse performance gains from inlining.

[benlesh]

You can't handle errors at the top level unless you keep a stack of observers to notify when errors propagate back up,

Technically, you only need the observer that triggered the error. Observers can push errors up through the chain above them on their own. @jhusain and I were able to hack together something viable (if hacky).

[benlesh]

The speed refactor stuff is extensive enough that we're going to need to punt those changes until v6. There's no reason to hold off on the 5.0 release for those changes. Removing "Blocking Five Oh Release" label.

[lock[bot]]

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.