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

Problems I see with the callback style #41

Closed
benlesh opened this issue Jul 9, 2015 · 57 comments
Closed

Problems I see with the callback style #41

benlesh opened this issue Jul 9, 2015 · 57 comments

Comments

@benlesh
Copy link

benlesh commented Jul 9, 2015

  1. Polyfill and library implementations will have to create a closure or use Function.prototype.bind to create the next, error and completed callbacks. This is because in order to create those callbacks and have them operate safely, they'll need to share some state. So either they have to live on the same instance of an object, or they need to close over shared variables.
  2. There's no straightforward way to short-circuit an unsubscription. If something down your observable chain asynchronously unsubscribes your observable, there's no boolean to check in your subscriber function (like an observer.isUnsubscribed) to know that you need to abort whatever loop you're in
@zenparsing
Copy link
Member

Polyfill and library implementations will have to create a closure or use Function.prototype.bind to create the next, error and completed callbacks

While creating various combinators, I found myself creating closures over the observer.next, etc. methods anyway, so I'm not totally convinced that using object actually reduces the number of closures in practice.

Is this a performance concern? Is it an irreducible complexity concern or just an optimization issue for current engines?

There's no straightforward way to short-circuit an unsubscription.

Not sure I understand this one. Can you explain a use case more concretely?

The previous generator API didn't have any such isUnsubscribed property, so it seems like you're wanting something that doesn't exist in either variation.

I think I may know what you're saying. Let's say we have this chain:

observable
-> combinatorA
  -> combinatorB
    -> observer

And the observer unsubscribes. That will cause the cleanup function for "combinatorB" to execute, which should then cause the cleanup function for "combinatorA" to execute, which should then cause the cleanup function for "observable" to execute. If observable's cleanup function sets a flag indicating that the observer has unsubscribed, then it will know to short-circuit any loops or whatnot.

Here's an example:

https://github.com/zenparsing/es-observable/blob/callback-style/src/Observable.js#L262

Does that cover the use case?

@benjamingr
Copy link

@Blesh

Polyfill and library implementations will have to create a closure or use Function.prototype.bind to create the next, error and completed callbacks.

I know I'm one of the people who keeps saying "treat performance only when it matters" and that everything needs to be measured, that said - I've found extra closure allocation to be an actual barrier in promises. Creating a closure each time isn't fun and it means observables come at a cost on top of stream APIs. It's a big part of why bluebird is still much faster than native promises, in turn native promises are working on a hook in browsers to facilitate a "shortcut" to fast conversion of APIs to address that (important work which I believe Domenic is heading).

Quoting Stef Panner:

"If the abstraction is as close to a zero cost abstraction, developers won't be tempted to short-circuit and absorb complexity themselves."

In terms of performance, I'm not sure what alternatives there are - but this would be a potential bottleneck for observables. Not that it's a huge deal, but it's worth noting.

@benlesh
Copy link
Author

benlesh commented Jul 9, 2015

@benjamingr the original proposal, that passes an observer with methods on it next, error and completed (or throw/return) into the subscriber function performs cleanly and doesn't require any closures in a solid implementation.


@zenparsing

While creating various combinators, I found myself creating closures over the observer.next, etc. methods anyway, so I'm not totally convinced that using object actually reduces the number of closures in practice.

I have not had to do any of this in ReactiveX/RxJS. I'm actually working off of an implementation @jhusain bestowed upon me... but pretty much anywhere that you'd have to create a closure, you can instead create a subclass of Observer and pass references that way. It performs much better.

The previous generator API didn't have any such isUnsubscribed property, so it seems like you're wanting something that doesn't exist in either variation.

Noted, that was something specific to my "safe observer" implementation. I guess my point is, if I have an object (observer) I'm passing into my subscriber function, I'm able to subclass that object and add implementation details to it like that.

FWIW, though, it should probably have an isUnsubscribed property for these purposes.

At the end of the day, if I'm tasked with creating a more performant version of RxJS, then meeting the callback-style spec will be a deal-breaker because of this.

@benjamingr
Copy link

@Blesh

the original proposal, that passes an observer with methods on it next, error and completed (or throw/return) into the subscriber function performs cleanly and doesn't require any closures in a solid implementation.

Can I get a side by side short example where a closure wasn't required but now is? I think I have a good idea how it'd look like var o = MyObserver(); observable.subscribe(o) vs o.subscribe(next => {}, err => {}, done => {})?

@zenparsing
Copy link
Member

@benjamingr With the callback style, the callbacks you pass to "subscribe" get wrapped in closures before being passed to the subscriber function (similar to the resolve and reject closures for promises). With the generator-style design, the observer you pass to "subscribe" is wrapped with an object that has methods.

@Blesh Can you show an example of a combinator where you are forwarding error and complete, that doesn't require the creation of a closure?

@trxcllnt
Copy link

Can you show an example of a combinator where you are forwarding error and complete, that doesn't require the creation of a closure?

@zenparsing I think @Blesh is talking about implementing operators in terms of Observer instead of Observables + closures. This is demonstrably faster than Observable + closures (and can be optimized further):

class Observable {
  constructor(subscribe) {
    if(subscribe) {
      this.subscribe = subscribe;
    }
  }
  subscribe(observer) {
    return this.source.subscribe(this.observerFactory.create(observer));
  }
  lift(observerFactory) {
    const o = new Observable();
    o.source = this;
    o.observerFactory = observerFactory;
    return o;
  }
  map(selector) {
    return this.lift(new MapObserverFactory(selector));
  }
}

class MapObserverFactory {
  constructor(selector) {
    this.selector = selector;
  }
  create(destination) {
    return new MapObserver(destination, this.selector);
  }
}

class MapObserver {
  constructor(destination, selector) {
    this.selector = selector;
    this.destination = destination;
  }
  next(x) {
    return this.destination.next(this.selector(x));
  }
  throw(e) {
    return this.destination.throw(e);
  }
  return(e) {
    return this.destination.return(e);
  }
}

@jhusain
Copy link
Collaborator

jhusain commented Jul 10, 2015

There are also legitimate ergonomic arguments for optionally accepting an object. It is very common to be an interested in any a subset of the callbacks, which means there is no one right order. As a result, developers will be forced to pass undefined in certain situations.

JH

On Jul 9, 2015, at 6:48 PM, Paul Taylor notifications@github.com wrote:

Can you show an example of a combinator where you are forwarding error and complete, that doesn't require the creation of a closure?

@zenparsing I think @Blesh is talking about implementing operators in terms of Observer instead of Observables + closures. This is demonstrably faster than Observable + closures (and can be optimized further):

class Observable {
constructor(subscribe) {
if(subscribe) {
this.subscribe = subscribe;
}
}
subscribe(observer) {
return this.source.subscribe(this.observerFactory.create(observer));
}
lift(observerFactory) {
const o = new Observable();
o.source = this;
o.observerFactory = observerFactory;
return o;
}
map(selector) {
return this.lift(new MapObserverFactory(selector));
}
}

class MapObserverFactory {
constructor(selector) {
this.selector = selector;
}
create(destination) {
return new MapObserver(destination, this.selector);
}
}

class MapObserver {
constructor(destination, selector) {
this.selector = selector;
this.destination = destination;
}
next(x) {
return this.destination.next(this.selector(x));
}
throw(e) {
return this.destination.throw(e);
}
return(e) {
return this.destination.return(e);
}
}

Reply to this email directly or view it on GitHub.

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

Yes. @trxcllnt is exactly right. This is what we're doing (roughly) for the next version of RxJS. It's a goal of RxJS Next to meet this spec, but performance is the primary goal, so any forced introduction of closure means we'll not be able to meet the spec.

@zenparsing
Copy link
Member

@jhusain We could allow an options bag as argument to subscribe, but that's not the issue. The issue is what is passed to the subscriber function.

@trxcllnt Gotcha - you're basically doing it "the hard way" to make JS engine optimizations happy.

I must say that the language designer part of me doesn't find these performance "deal-breaker" arguments very convincing. Providing callbacks to the subscriber is generally more ergonomic and is consistent with the design choices made for promises. I know, you all have a thing against promises.

Le sigh...

@benjamingr
Copy link

@zenparsing I don't have a thing against promises obviously :D

@jhusain
Copy link
Collaborator

jhusain commented Jul 10, 2015

@zenparsing assuming by the Subscriber function, you mean [Symbol.observer]? That's what I meant, my apologies for the confusion. I have been assuming that subscribe will have to be synchronous, but I know we haven't resolved that issue yet.

@trxcllnt can you come up with an example in which it is more ergonomic to use named parameters then callbacks? I know there have been situations in the past in which I have omitted the first or second call back, and then forced to pass undefined. Having some difficult coming up with an example.

With regard to Promises, I think there is a higher bar for the performance of stream types than for scalar types.

Let me propose the following principle: If someone wants to argue that performance can be improved by browser optimization, the burden should be on them to show this is possible, or at the very least present a well-considered proposal for how this might be done. A chrome branch would do for example. Browsers have had ample opportunity and incentive to optimize closures. They are not a new construct. I'm very wary of claims that compilers can optimize away implementation costs when they are not accompanied by any evidence or at least a proposal.

@Blesh, is there anyway we can get performance numbers contrasting each approach in the three major browsers today? It would be interesting to see whether there were variances or all major browsers were slower with closures.

@benjamingr
Copy link

@jhusain

With regard to Promises, I think there is a higher bar for the performance of stream types than for scalar types.

Actually, I find quite the opposite to be the case. Since promises represent singular values you can easily have 50000 of them in an application so creation has to be very fast. With streams I usually have a few observables I created ahead of time which I use for data throughout the app lifecycle, so observable creation is at most as common as promise creation IMO.

As for optimization of closures, it's possible to solve this for native observables through "private symbols" as is done for promises.

@jhusain
Copy link
Collaborator

jhusain commented Jul 10, 2015

Thanks benjamin. I was not aware of that Promises were optimized in that way. That's should produce the same outcome as creating an object manually.

@benlesh Seems like the RxJS library could just use an internal subscribe method that accepts an options bag, but have the external API accept callbacks. In fact I'm pretty sure that's how it works today.

JH

On Jul 10, 2015, at 8:52 AM, Benjamin Gruenbaum notifications@github.com wrote:

@jhusain

With regard to Promises, I think there is a higher bar for the performance of stream types than for scalar types.

Actually, I find quite the opposite to be the case. Since promises represent singular values you can easily have 50000 of them in an application so creation has to be very fast. With streams I usually have a few observables I created ahead of time which I use for data throughout the app lifecycle, so observable creation is at most as common as promise creation IMO.

As for optimization of closures, it's possible to solve this for native observables through "private symbols" as is done for promises.


Reply to this email directly or view it on GitHub.

@benjamingr
Copy link

@jhusain I do think that in addition to demonstrating capability to model various APIs in browsers (and optionally in Node although that carries less weight) - we need to think about fast observable creation.

In general, I think that:

  • Performance should not be a consideration for callback vs object at this stage. Both can have platform level solutions - the more ergonomic and simple API is important here.
  • Once the API is good, we can optimize pretty heavily. Much better than current libraries (*).
  • I regret bringing up performance here and apologize for doing so, my intention was to raise discussion of how to make it fast later by hooks and not to change the API for performance.

(*) A few words on new Observable speed:

tl;dr current libraries do a much much slower job than the potential of either proposal anyway and we're enjoying them just fine - so this shouldn't be a blocker.

Longer version:

For what it's worth current observable libraries do a very slow job at converting APIs to observables or the observable constructor. Let's take RxJS for instance since it's the one both of us use for our production code. RxJS is very naive at the moment at observable construction. It does big no-nos like var self = this; and uses bind in the constructor. It doesn't extract try/catch out there and it generally does a pretty slow job at construction.

To make things worse, methods like fromEvent are very slow, they allocate a closure and wrap the existing addEventListener, a closure is used in the hot next path and so on. fromNodeCallback is equally bad, it allocates a closure and is very slow compared to how it can be. Rx-Node makes similar naive optimizations. Just to contrast, bluebird dynamically compiles a JIT friendly function for callbacks, and does not allocate any closures.

Now, this is all fine because in RxJS, I can return promises to observables like in flatMap in Node and it works quite well. As I said, in RxJS I typically allocate a few (let's say 50) observables at the start and keep flowing everything through them. For example if I have a "latest posts" observable that returns the latest posts in a news site from a data source and every request uses it, with promises - every request would need to be a new promise - so bluebird has to be fast in observable creation and RxJS doesn't (unless you start using observables to singulars on the server).

Just please - let's not change APIs for performance just yet.

An Observable.from method on NodeJS

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

From a conversation with @jhusain:

I think that this thread has misunderstood what I'm saying: This has nothing to do with subscribe.

I was speaking specifically about the constructor signature of: constructor(nextCallback, errorCallback, completedCallback). Each one of those callbacks have a shared state, in that you're not allowed to call nextCallback if completedCallback has been called. Also there's the underlying state of whether or not the subscription has been disposed, you can't call nextCallback if everything has been disposed.

That means that in order to create these callback functions to hand to the constructor, as an implementor, I'll be forced to either close over some shared state (an isUnsubscribed for example), or I'll need to create an Observer instance under the hood, and expose it's next, throw and return methods by calling Function.prototype.bind on them.

Does that add more clarity to the issue I'm raising?

@benjamingr
Copy link

@Blesh I don't think anyone misunderstood this as being about subscribe, we are talking about new Observable potentially requiring a closure. My point is that we're (RxJS users here) all pretty OK with the speed things work in RxJS and it doesn't perform even the most basic performance optimizations for the cases we're discussing here - so maybe we don't have a perf issue yet?

I've added some examples in my last comment of very low hanging fruit to make new Observable (as well as fromNodeCallback and fromEvent) significantly lower in overhead - I'm just not actually creating tens of thousands of observables in my app (unlike promises) - something Jafar's comment reminded me.

Client side overhead of wrapping events is pretty mild so let's discuss "more pressuring" situations like servers. You have a news site, whenever a user accesses the home page you first check if it is cached to REDIS, if it is you fetch it from there, if it's not you make a DB request, when that arrives you process it - store it in REDIS and send the result to the user, in addition you want to report to an analytics service that the request arrived.

  • Promises - Since promises are singular - a promise is "single use" per request. A promise API over that would require promises for the redis fetch, db fetch, redis store, analytics call and recovery from REDIS - in addition all those would probably use .all at some place and thenable chaining - so it's very realistic to have 20 promises per request here. If you have 5000 connected clients you get a total of 100000 promises. For these scenarios promises need to be very fast to create and indeed 100000 promises is something bluebird will eat for breakfast - so it's not a performance problem with fast promise libraries but it would be one if promises were slow to make.
  • Observables - I'd have a single observable for fetching the front page from REDIS, a single observable for DB requests for that page - it would be chained so that's an additional observable for storing the data in REDIS if fetched from the db, one more Subject for the Analytics service. Overall less than 20 observables for the entire lifespan of my server - even if I can make only 100 observables per second that's fine because they live for much much much longer. This all changes if you use observables for singular values instead of promises, I realized that and opened an issue at fromNodeCallback should be fast Reactive-Extensions/RxJS#809 , but if promises solve it with that API, so can observables - since the numebr of observables on-the-fly is lower in potential.

@zenparsing
Copy link
Member

That means that in order to create these callback functions to hand to the constructor, as an implementor, I'll be forced to either close over some shared state (an isUnsubscribed for example), or I'll need to create an Observer instance under the hood, and expose it's next, throw and return methods by calling Function.prototype.bind on them.

Yep. Current engines do a horrible job of optimizing bind - it's terribly slow. My hope (and I don't have the domain knowledge of engine internals to say much more) is there is room to further optimize bound functions in certain cases (like when we are just binding a this value).

If we move function bind syntax forward, engines will have a good deal of incentive to optimize such bound functions. I'm optimistic.

But I think there are optimization opportunities for libraries that want to eliminate closures. Can you add an Rx extension which takes a object-accepting subscriber and fast path that?

Rx.makeFastObservable = function(subscriber) {
    // NOTE: "subscriber" accepts an object, not 3 callbacks
    // ...
};

Rx.subscribeWithObserver = function(observable, observer) {
    // Fast path
    if (observable[Rx.subscribeWithObserver]) {
        return observable[Rx.subscribeWithObserver](observer);
    }
    // Slow path - simplified
    return observable.subscribe(
        x => observer.onNext(x),
        x => observer.onError(x),
        x => observer.onComplete(x));
};

Rx.Observable.prototype.map = function(fn) {
    return Rx.makeFastObservable(observer => {
        return Rx.subscribeWithObserver(this, new Rx.MapObserver(observer, fn));
    });
};

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

@zenparsing Rx has that with Observable.create technically, but I honestly wouldn't even want to give people the option to use new Observable((next, error, completed) => {}) because they'll shoot themselves in the foot. JS developers that are moving over from C# and Java just love new-ing up things. I would, in fact, be tempted to add warnings to console in development builds like "don't use 'new Observable', it's bad".

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

It seems like this API design is meant to mirror the design of new Promise((resolve, reject) => {})... but Promise is a different animal. resolve and reject are finalizing calls, they don't need to share state about one another, really. In Observable, the next, error and completed callbacks handed to the constructor are fundamentally linked. I think it should be one thing, an Observer.

@zenparsing
Copy link
Member

@Blesh

JS is not C# or Java. At least from an API point of view, closures are a good thing in JS. In many cases it is much more pleasant to create closures rather than new-ing up types. I find this idea that we must avoid closures in JS API design quite strange, actually. Especially since you seem to agree that Rx can optimize it's internal combinators to use objects and methods.

I honestly wouldn't even want to give people the option to use new Observable((next, error, completed) => {}) because they'll shoot themselves in the foot

Shoot themselves in the foot by using closures, in JS? Really? : )

resolve and reject are finalizing calls, they don't need to share state about one another

Well, they have to point to the same promise object. Note that the closures created in the callback style don't need to reference each other either. They need to reference the subscription which holds references to the original callbacks provided to subscribe.

Mirroring the design of Promise is not essential, but it's a really nice outcome. It's a good thing when there is consistency between APIs of the standard library.

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

I just think this design isn't something we can implement in JS without a bind or closure:

The crude code I typed up below should illustrate the problem...

class Observable {
  constructor(subscriber) {
    this.subscriber = subscriber;
  }

  subscribe(next, error, completed) {
    var observer = new Observer(next, error, completed);

    // ***** HERE there be bind! Yarr! ****/
    var disposalAction = this.subscriber(
      observer.next.bind(observer), 
      observer.error.bind(observer), 
      observer.completed.bind(observer));

    return new Subscription(disposalAction, observer);
  }
}

class Observer {
  isUnsubscribed = false;

  constructor(next, error, completed) {
    this._next = next;
    this._error = error;
    this._completed = completed;
  }

  next(x) {
    if(!this.isUnsubscribed) {
      this._next(x);
    }
  }

  error(x) {
    if(!this.isUnsubscribed) {
      this._error(x);
      this.unsubscribe();
    }
  }

  completed(x) {
    if(!this.isUnsubscribed) {
      this._error(x);
      this.unsubscribe();
    }
  }

  unsubscribe() {
    this.isUnsubscribed = true;
  }
}

class Subscription {
  constructor(action, observer) {
    this.action = action;
    this.observer = observer;
  }

  dispose() {
    if(!this.isDisposed) {
      if(this.observer) {
        this.observer.unsubscribe();
      }
      if(this.action) {
        this.action();
      }
      this.isDisposed = true;
    }
  }
}

I'd challenge anyone to implement this design (new Observable((next, error, completed) => {})) without closure or function.bind, while maintaining the safety of the type (next can't be called after disposal or completion, etc)

Unless we can do that, I think there's a flaw in this design, and I really can't implement it, as specified, in RxJS Next. I'll be happy if I'm wrong, because I think this is an ergonomic design. I just think it's going to miss the mark for the ReactiveX community, performance-wise.

As a side note: It's also less extensible, because you can more easily add functionality (in a later spec) to the Observer your passing in, than you can to three functions.

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

I really hope my last comment illustrates what I'm saying because I feel like I'm being misread over and over. LOL

@zenparsing
Copy link
Member

I'd challenge anyone to implement this design (new Observable((next, error, completed) => {})) without closure or function.bind, while maintaining the safety of the type (next can't be called after disposal or completion, etc)

Right - you have to use a closure or bind. What I'm saying is that for your internal implementation of map, groupBy, etc, I think you can use an optimized object-observer and fast-path it. See #41 (comment) .

I just think it's going to miss the mark for the ReactiveX community, performance-wise.

Well, let's assume that you've implemented the kind of fast-path I describe (and let me know if I'm totally off the mark). Then what kind of performance problem are we left with? Is there any way to quantify it or make it more concrete?

As a side note: It's also less extensible, because you can more easily add functionality (in a later spec) to the Observer your passing in, than you can to three functions.

Noted.

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

Well, let's assume that you've implemented the kind of fast-path I describe

We already are, actually. So this part of the problem might not be so bad.

There's another interesting problem I'm going to face due to the extensibility issue though, best illustrated by range:

Observable.range = function(start, end) {
  return new Observable((next, error, completed) => {
    var i;
    for(i = start; i < end; i++) {
      next(i);
    }
    completed();
  });
};

Observable.range(0, Number.MAX_VALUE).take(5).subscribe(x => console.log(x));

vs

Observable.range = function(start, end) {
  return new Observable((observer) => {
    var i;
    for(i = start; observer.isUnsubscribed && i < end; i++) {
      observer.next(i);
    }
    observer.completed();
  });
};

Observable.range(0, Number.MAX_VALUE).take(5).subscribe(x => console.log(x));

In the two examples above, one is going to complete the entire loop before finishing, the other isn't, because I'm able to track, on my observer implementation, whether or not I've been unsubscribed.

** I realize the argument here is "but there's no isUnsubscribed on observer **

Ultimately, I can create a special "RangeObservable" that has already implemented it's own subscriber function and only deals with my observers, and work around the ES7 Observable constructor. But do we want to produce a constructor we have to work around?... if there's literally nowhere in RxJS Next's implementation I can use your constructor, is it really the right constructor?

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

How can we propose an Observable type without an Observer type? It just seems odd. Observation has defined behaviors that the Observer embodies. I don't think that should be abstracted away just to make it more promise-like. We miss out on use cases that could be handled by subclassing Observer. It's so tried and true, I don't think we should be reinventing it.

@zenparsing
Copy link
Member

@Blesh Copy-pasted from another thread:

Observable.range = function(from, to) {

    return new Observable((next, error, complete) => {

        let stop = false;

        enqueueJob(_=> {
            for (let i = from; i <= to && !stop; ++i)
                next(i);
            complete();
        });

        return _=> stop = true;
    });
};

The cleanup function takes care of this, no? (Ignore the enqueueJob thing for now...)

But do we want to produce a constructor we have to work around?... if there's literally nowhere in RxJS Next's implementation I can use your constructor, is it really the right constructor?

You only have to work around it until JS engines optimize those closures/bindings/whatever. Same issue as Promises, true?

Worse case is that engines never optimize it. Best case is they do. In this discussion we've produced no analysis on:

  • What the difference in performance is between these two extremes
  • What the likelihood of either extreme is

Until we do some analysis, we ought not jump to conclusions; we should keep an open mind.

Unfortunately, I don't have a clue how to answer either question right now. I'll put some thought into it though... In the meantime, let's set aside the "deal-breaker" performance FUD : P

How can we propose an Observable type without an Observer type

I think that's a very C# way of looking at things. C# is great, but it's not JS. The real advantage of callbacks is that you can implement any observer-ish thing on top of it. That was the whole point in my mind; we get to side-step the generator vs. observer swamp, since either type can be implemented easily on top of callbacks.

@benjamingr
Copy link

@Blesh

I'd challenge anyone to implement this design (new Observable((next, error, completed) => {})) without closure or function.bind, while maintaining the safety of the type (next can't be called after disposal or completion, etc)

I always try to pick up challenges. No observers and no bind or closures, just next here but same idea applies to done and errors:

class Observable {
     constructor(executor){
          let next = function onNext(v) {  // function name is different just to show there is no closure
              onNext.context.subscriber(v);
          };
          next.context = this; // note no bind, or closure, since the name is local to the function
          executor(next);
     }
     subscribe(onNext){
          this.subscriber = onNext;
     }
}
Observable.prototype.subscriber = () => {};

And on Babel:

http://babeljs.io/repl/#?experimental=true&evaluate=true&loose=false&spec=false&playground=false&code=class%20Observable%20%7B%0A%20%20%20%20%20constructor(executor)%7B%0A%20%20%20%20%20%20%20%20%20%20let%20next%20%3D%20function%20onNext(v)%20%7B%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20onNext.context.subscriber(v)%0A%20%20%20%20%20%20%20%20%20%20%7D%3B%0A%20%20%20%20%20%20%20%20%20%20next.context%20%3D%20this%3B%20%2F%2F%20note%20no%20bind%2C%20or%20closure%2C%20since%20the%20name%20is%20local%20to%20the%20function%0A%20%20%20%20%20%20%20%20%20%20executor(next)%3B%0A%20%20%20%20%20%7D%0A%20%20%20%20%20subscribe(onNext)%7B%0A%20%20%20%20%20%20%20%20%20%20this.subscriber%20%3D%20onNext%3B%0A%20%20%20%20%20%7D%0A%7D%0AObservable.prototype.subscriber%20%3D%20()%20%3D%3E%20%7B%7D%3B%20%2F%2F%20default%20to%20noop%0A%0Alet%20o%20%3D%20new%20Observable(next%20%3D%3E%20%7B%0A%20%20let%20i%20%3D%200%3B%0A%20%20var%20interval%20%3D%20setInterval(_%20%3D%3E%20%7B%0A%20%20%20%20if(i%20%3E%2010)%20return%20clearInterval(interval)%3B%0A%20%20%20%20next(i%2B%2B)%3B%0A%20%20%20%20%0A%20%20%7D%2C%201000)%3B%0A%7D)%3B%0A%0A%0Ao.subscribe(i%20%3D%3E%20console.log(i))%3B%0A

@benjamingr
Copy link

But really, there are like 4 different ways to do this (including the one above), and the one bluebird does is much faster so you might want to check that one out. Promises either take an executor or a special INTERNAL symbol - if they get INTERNAL they don't do a lot of work or create a closure. One example is in the promsiification code (the code I've already linked to).

Anyway - to reiterate - we should not be talking about performance issues in cases like this since Rx has acceptable performance and it's not optimized for this case at all. I'm not sure why when I say that I get ignored but this is a problem with promises (creation time) and not with observables.

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

I always try to pick up challenges. No observers and no bind or closures, just next here but same idea applies to done and errors:

Your example is inadequate. The whole point is the interaction between next, error and complete... For example, you shouldn't be able to call next after complete or call complete twice in a row and have those things emit.

You can't give next a context of the Observable it's observing, because you need to create N next functions for N subscribe calls.

To reiterate, you need to make this safe:

var test = new Observable((next, throw, complete) => {
    next(1); 
    next(2);
    complete();
    next(3);
});

test.subscribe(x => console.log(x), null, () => console.log('done'));

/*
1
2
"done"
*/

I think you might be able to use the same technique you're trying to use by treating the next function has a hash table and setting context to something you new-up or an object literal, but you're still creating a closure in doing so, in that you're closing over the variable you're setting your putting your function in.

@benjamingr
Copy link

@Blesh of course it was "inadequate", I greatly simplified it (no done even). I was merely demonstrating that there are ways to get around allocating a closure or using bind - namely by using the fact a function can reference its own name.

Here, just for kicks I did done with a different technique (using the fact arrow functions have bound context but don't bind, in v8 this isn't slow if I understand the code correctly - but you can always put a parameter on the function object instead if you like that better):

http://babeljs.io/repl/#?experimental=true&evaluate=true&loose=false&spec=false&playground=false&code=class%20Observable%20%7B%0A%20%20%20%20%20constructor(executor)%7B%0A%20%20%20%20%20%20%20%20%20%20let%20next%20%3D%20function%20onNext(v)%20%7B%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20onNext.context.subscriber(v)%0A%20%20%20%20%20%20%20%20%20%20%7D%3B%0A%20%20%20%20%20%20%20%20%20%20let%20done%20%3D%20()%20%3D%3E%20delete%20this.subscriber%3B%0A%20%20%20%20%20%20%20%20%20%20next.context%20%3D%20this%3B%20%2F%2F%20note%20no%20bind%2C%20or%20closure%2C%20since%20the%20name%20is%20local%20to%20the%20function%0A%20%20%20%20%20%20%20%20%20%20executor(next%2C%20done)%3B%0A%20%20%20%20%20%7D%0A%20%20%20%20%20subscribe(onNext)%7B%0A%20%20%20%20%20%20%20%20%20%20this.subscriber%20%3D%20onNext%3B%0A%20%20%20%20%20%7D%0A%7D%0AObservable.prototype.subscriber%20%3D%20()%20%3D%3E%20%7B%7D%3B%20%2F%2F%20default%20to%20noop%0A%0Alet%20o%20%3D%20new%20Observable((next%2C%20done)%20%3D%3E%20%7B%0A%20%20let%20i%20%3D%200%3B%0A%20%20setTimeout(_%20%3D%3E%20%7B%20%0A%20%20%20%20next(1)%3B%0A%20%20%20%20next(2)%3B%0A%20%20%20%20done()%3B%0A%20%20%20%20next(3)%3B%0A%20%20%7D)%0A%7D)%3B%0A%0A%0A%0A%0Ao.subscribe(i%20%3D%3E%20console.log(i))%3B%0A

@benjamingr
Copy link

Also note my code doesn't actually implement observables (it's not lazy, it runs the executor in the constructor and not on subscribe) - I'm just demonstrating a technique to do something.

I think you might be able to use the same technique you're trying to use by treating the next function has a hash table and setting context to something you new-up or an object literal, but you're still creating a closure in doing so, in that you're closing over the variable you're setting your putting your function in.

function noClosure(){ 
    return function foo(){ // I'm not even making a reference to foo anywhere
        foo.bar();
    }
}
var bar = noClosure();
bar.bar = () => console.log("HI");
bar(); 

Do we agree there is no closure in this case?

@benlesh
Copy link
Author

benlesh commented Jul 10, 2015

I think that's a very C# way of looking at things

@zenparsing Let's be fair, it's a very "This is been implemented dozens of times" way of looking at things. This is a pattern that's tried and true. I think that the new pattern will work, but I think there are flaws in comparison to the original pattern, flaws I've outlined above. This has nothing to do with my thought process being rooted in any other languages, and saying that paints my arguments unfairly.

The real advantage of callbacks is that you can implement any observer-ish thing on top of it. That was the whole point in my mind; we get to side-step the generator vs. observer swamp, since either type can be implemented easily on top of callbacks.

But it's three functions? You can't extend an object to be three functions, but you can add three methods to an object. Unless I'm missing something?

I think the only swamp we were in was the Generator swamp. The Observable/Observer pattern is well established.

Rx has acceptable performance

@benjamingr Current RxJS, in fact, does not have adequate performance. Not for Netflix's needs at least. It's implementation has a lot of overhead in the form of closures and some over-zealous scheduling it borrowed from the .NET implementation of Rx. This is why the community is rewriting RxJS.

I was merely demonstrating that there are ways to get around allocating a closure

@benjamingr but your example does introduce closure, it closes over onNext which allocated in the outer function scope. function foo() { foo.anything; //<--closure } ... if you didn't var,const or let it inside of your function, it's closing over something.

@benjamingr
Copy link

It's not closing over anything, it's referencing itself. Note that the outside environment has no reference to any identifier being used. Would using arguments.callee make the fact there is no closure more explicit?

@benjamingr Current RxJS, in fact, does not have adequate performance. Not for Netflix's needs at least. It's implementation has a lot of overhead in the form of closures and some over-zealous scheduling it borrowed from the .NET implementation of Rx. This is why the community is rewriting RxJS.

Well good luck - I'm rooting for it. If I may offer one piece of advice? Hire an ex v8 compiler engineer or someone who is extremely knowledgable of how the JIT works and how to benchmark to work with you on it - performance is hard, really hard, and the only way to get someone like Petka to write "bluebird for RxJS" is to either pay them a lot of money or convince them it's worth their time. There are probably less than 10 people like that but you only need one. Matthew is awesome, and RxJS is awesome, but it wasn't written to be performance oriented in the first place and if you're rewriting it you need to be very aware of it from the start.

@benjamingr
Copy link

@Blesh I think the confusion came from the fact I used a named function expression. To clarify:

(function foo(){ foo.toString(); }); // function expression, not a function declaration.
foo; // ReferenceError: foo is not defined

@jhusain
Copy link
Collaborator

jhusain commented Jul 10, 2015

This certainly seems to solve the issue. Embarrassed that I didn't consider a named function expression. Thanks again Benjamin! I'm sure that @Blesh will performance test this approach and we can put this issue to rest.

JH

On Jul 10, 2015, at 4:28 PM, Benjamin Gruenbaum notifications@github.com wrote:

@Blesh I think the confusion came from the fact I used a named function expression. To clarify:

(function foo(){ foo.toString(); }); // function expression, not a function declaration.
foo; // ReferenceError: foo is not defined

Reply to this email directly or view it on GitHub.

@benlesh
Copy link
Author

benlesh commented Jul 11, 2015

@benjamingr oh, it's a named function. That's interesting, but I'm still unsure if that does or doesn't constitute "closing over" something or any additional allocation. I'm also uncertain if using a Function as a hash table will cause deoptimization in V8 (or other engines).

We'll experiment with this approach, of course, if the real decision is to toss the Observer type out altogether. That scares me a little because it means we're producing an Observable type that is untested. All-new edge-cases to takckel

@benjamingr
Copy link

That scares me a little because it means we're producing an Observable type that is untested. All-new edge-cases to takckel

That's the point :) To list things that bother us and tackle them. There are ways other than the named function trick above, I was just facing your challenge. I think we can agree performance shouldn't be our focus for this proposal right now as both techniques can be optimized beyond any reasonable barrier.

Named parameters with default values would make me like the current API more - but even without them in JS it looks nicer than the old observer API so far. Haven't written anything real in it yet though so I guess I'll find out.

@benlesh
Copy link
Author

benlesh commented Jul 11, 2015

I think we can agree performance shouldn't be our focus for this proposal right now

I'm more concerned, personally, about performance in RxJS Next. Which has two goals: performance improvements and meeting this spec. But meeting this spec is the least important of those two goals, especially if it's at odds with performance.

@benlesh
Copy link
Author

benlesh commented Jul 12, 2015

Preliminary tests show definitively that using a named function as a hash table to reference variables inside of itself, does not cause the function to deoptimize.

It also showed that closures do not constitute deoptimization.

I'm still not entirely convinced that the pattern proposed by @benjamingr doesn't create a closure.

Also the implementation is looking extremely ugly as I'm trying to convert RxJS to this paradigm, which I'm aware this spec doesn't care about, but it probably a sign we're not doing the right thing. I strongly disagree with tossing out Observer, which is a useful abstraction on its own.

@trxcllnt
Copy link

That scares me a little because it means we're producing an Observable type that is untested.

That's the point :) To list things that bother us and tackle them.

@Blesh's point is that until we have a reference implementation, we're making decisions in a vacuum. It's Netflix's opinion that the specification should be based on working code. The mathematics of the Observable type are strictly defined, and can't necessarily change to fit the stylistic whims of a few of the language's users.

For example, lift :: Observable -> (Observer -> Observer) -> Observable can't be redefined to operate over three callbacks. This is enough to reject this idea.

@zenparsing
Copy link
Member

@Blesh

Also the implementation is looking extremely ugly as I'm trying to convert RxJS to this paradigm

This seems interesting; is there any way to see before-and-after code?

@trxcllnt

For example, lift :: Observable -> (Observer -> Observer) -> Observable can't be redefined to operate over three callbacks. This is enough to reject this idea.

All this shows is that a particular type signature is not possible if we eliminate one of the types, which is tautological. Can you show an example where this "lift" is useful but not possible with the callback API?

@trxcllnt
Copy link

@zenparsing the lift discussion has been started at ReactiveX/rxjs#60, but I'll elaborate more here.

All Observable operators can be implemented in terms of lift, which has the following advantages:

  1. reduces the subscription call-stack
  2. enables the library to factor out closures in favor of classes, which are already optimized by the major engines
  3. isolates Observable creation to lift
    • The default implementation is likely going to create intermediate Observables with Object literals and __proto__ assignment, as this has proven to be significantly faster in JIT and non-JIT environments alike.
    • allows developers to subclass Observable for extension, instrumentation, or debugging without the need to override every operator.
But doesn't the new function bind operator (::) obviate the need to extend Observable?

No. While the function bind operator allows us to invoke methods that operate on Observable in a fluent style, lift allows the developer to change what kind of Observable is returned.

For example, we could write a Subject that communicates bidirectionally with a WebSocket. All operations performed on the Subject would yield a new Subject, maintaining the bidirectional communication between the source and the subscription:

class Observer {
  constructor(destination) {
    this.destination = destination;
  }
  create(destination) {
    return {
      destination: destination,
      __proto__: Observer.prototype
    };
  }
  next(x) {
    return this.destination.next(x);
  }
  throw(e) {
    return this.destination.throw(e);
  }
  return() {
    return this.destination.return();
  }
}

class MapObserver extends Observer {
  constructor(destination, selector) {
    this.selector = selector;
    super(destination);
  }
  create(destination) {
    return new MapObserver(destination, this.selector)
  }
  next(x) {
    return super.next(this.selector(x));
  }
}

class Observable {
  constructor(subscribe) {
    this.subscribe = subscribe;
  }
  lift(observerFactory) {
    // dirty but fast
    return {
      source: this,
      observerFactory: observerFactory,
      __proto__: Observable.prototype
    };
  }
  subscribe(observer) {
    return this.source.subscribe(this.observerFactory.create(observer));
  }
  map(selector) {
    return this.lift({ selector: selector, __proto__: MapObserver.prototype });
  }
}

Observable.prototype.observerFactory = Observer.prototype;

class TwoWaySubject extends Observable {
  constructor(observable, observer) {
    this.source = observable;
    this.destination = observer;
  }
  lift(observerFactory) {
    return {
      source: this,
      destination: this.destination,
      observerFactory: observerFactory,
      __proto__: TwoWaySubject.prototype
    }
  }
  next(x) {
    return this.destination.next(x);
  }
  throw(e) {
    return this.destination.throw(e);
  }
  return() {
    return this.destination.return();
  }
}

class WebSocketSubject extends TwoWaySubject {
  constructor(url) {
    this.url = url;
    this.pendingMessages = [];
    super(this, this);
  }
  subscribe(observer) {

    this.socket = new WebSocket(url);

    this.socket.onopen = () => {
      const pendingMessages = this.pendingMessages.slice(0);
      this.pendingMessages.length = 0;
      pendingMessages.forEach((x) => { this.next(x); });
    }
    this.socket.onmessage = (x) => { observer.next(JSON.parse(x.data)) };
    this.socket.onerror = (e) => { observer.throw(e) };
    this.socket.onclose = (x) => {
      observer.next(x);
      observer.return();
    }

    return {
      unsubscribe: () => {
        this.socket.onopen = null;
        this.socket.onmessage = null;
        this.socket.onerror = null;
        this.socket.onclose = null;
        this.socket.close();
        this.socket = null;
      }
    };
  }
  next(x) {
    if(this.socket) {
      if(this.socket.readyState === 1) {
        this.socket.send(JSON.stringify(x));
      }
    } else {
      this.pendingMessages.push(x);
    }
  }
  throw(e) {
    if(this.socket) {
      this.socket.close(e);
    }
  }
  return() {
    if(this.socket) {
      this.socket.close();
    }
  }
}

class EventSubject extends TwoWaySubject {
  constructor(type, observable, observer) {
    this.type = type;
    super(observable, observer);
  }
  lift(observerFactory) {
    return {
      type: this.type,
      source: this,
      destination: this.destination,
      observerFactory: observerFactory,
      __proto__: EventSubject.prototype
    }
  }
  next(x) {
    return super.next({ type: this.type, value: x });
  }
}

var socket = new WebSocketSubject("ws://127.0.0.1:10101/");
var events = socket
  .groupBy((x) => { x.type; })
  .map((xs) => { new EventSubject(xs.key, xs, socket); });

events.subscribe((eventsByType) => {
  eventsByType.subscribe((event) => {
    if(event.someValue > 10) {
      // sends a message back to the WebSocket with the event's "type" attached
      eventsByType.next("some value is greater than 10!");
    }
  });
});

@benjamingr
Copy link

@trxcllnt

For example, we could write a Subject that communicates bidirectionally with a WebSocket. All operations performed on the Subject would yield a new Subject, maintaining the bidirectional communication between the source and the subscription:

Just want to point out that :: functions can do this just fine by invoking the .constructor of the object - if the need arises and that's insufficient the Symbol.species pattern can be utilized as it is in other parts of the language (like arraysublass.map(fn) returns an arraysublass instance)

@Blesh

I'm still not entirely convinced that the pattern proposed by @benjamingr doesn't create a closure.

Nothing lexical is being captured from the external scope. Also, I'm not sure how many times I can repeat this but here goes n+1: doing the fn.context trick was just to solve your challenge - I do not recommend actually using it in RxJS. As for performance.

RxJS is not slow because of the API. RxJS is slow because it's written in pretty unoptimized JavaScript (and again, I've never had performance issues with RxJS myself and we use it all the time at TipRanks). My original reply to @jhusain details some trivial examples #41 (comment) but really it's much more than that.

I strongly disagree with tossing out Observer, which is a useful abstraction on its own.

Then make a compelling argument using real code samples illustrating the difficulty here (a gist would be nice) - anything but performance is worth considering.

@zenparsing
Copy link
Member

@trxcllnt Thanks, I'll need some more time to fully digest but this is helpful.

reduces the subscription call-stack

I have a hard time believing this is an actual problem. What evidence is there? Furthermore, the callback API seems to give the same call stack height as your proto-swizzling solution (I'll post some analysis later.)

enables the library to factor out closures in favor of classes, which are already optimized by the major engines

Not sure I buy this either. Didn't @Blesh just say that using closure doesn't cause significant de-opt?

The default implementation is likely going to create intermediate Observables with Object literals and proto assignment, as this has proven to be significantly faster in JIT and non-JIT environments alike.

Proto assignment is generally regarded as a hack best avoided; it certainly has no place in the standard library. I really feel like you are "doing it the hard way" and using closures is much easier. I'll expand on that when I get a chance to sit down and code.

As far as performance claims go, "faster" is meaningless to me. Faster than what? How much faster? On what engines, etc?

allows developers to subclass Observable for extension, instrumentation, or debugging without the need to override every operator.

Observable (as I've defined it) is designed to be subclassable, so I'm not sure what lift has to do with it? Check out https://github.com/zenparsing/es-observable/blob/callback-style/src/Observable.js#L340 for a subclass-friendly "map" combinator, no "lift" required.

@jhusain
Copy link
Collaborator

jhusain commented Jul 12, 2015

@benjamingr @zenparsing A few questions for you. Does it concern you that native Promises are sufficientl6y slow that third-party libraries like Bluebird have to replace them entirely? Wouldn't it be better to have a native Promise that was fast? Don't browsers already have ample incentive to optimize bind? Do you perceive that the :: operator (which I'm a big fan of) will make it easier to optimize bind, or merely provide additional incentive by spreading the pattern? Would you agree that it might be valuable to get feedback from browser implementers?

@benjamingr Convinced you've gotten rid of the closure. Seems like attaching a property to a function might cause deoptimization, but I need to pull up D8 and confirm that. You claim there are other solutions to avoiding closure creation and I'm sure your right given the versatility of JS. It seems promisify generates one closure to bind the callback and promise together so that the Promise can be marked resolved once the callback completes.

https://github.com/petkaantonov/bluebird/blob/master/src/promise_resolver.js#L37

Obviously this is better than three closures, but are there any other ideas you'd be be willing to share?

@trxcllnt do you acknowledge that :: allows higher-kind polymorphism via this.constructor?

@Blesh Doesn't @zenparsing's fast path solution completely resolve perf problems associated with observer creation? All RxNext Observables can implement subscribeWithObserver and presumably the current implementation doesn't need to change much.

My feeling is that the observer abstraction will be hard sell here based on precedent with Promises. Would like to not have to work around the native Observable type, but I acknowledge that this works in the Promise world and the planned improved debugging extensibility should work equally well for third-party Observables.

@benlesh
Copy link
Author

benlesh commented Jul 12, 2015

Nothing lexical is being captured from the external scope

Except that named function created in the external scope. I'm sorry, I don't want to just take your word for it.

RxJS is not slow because of the API. RxJS is slow because it's written in pretty unoptimized JavaScript (and again, I've never had performance issues with RxJS myself and we use it all the time at TipRanks).

@jhusain, @trxcllnt and I can tell you exactly why current RxJS is slow. What we've found through extensive performance testing is this is a small portion of it. The environments we need this to be fast in for Netflix are unlucky anything you're going to run into anywhere else. Non-JITTed, resource-constrained hardware, and Node servers at massive scale. I assure you, there is sound reasoning behind why we're interested in a faster RxJS.

Then make a compelling argument using real code samples illustrating the difficulty here (a gist would be nice) - anything but performance is worth considering.

There will be code examples forthcoming, in the form of an altered branch of RxJS Next. I was already planning on this. But as I've stated "n+1" times: if this design affects performance in RxJS Next, we can't pursue it in that library. Which will be disappointing, because it's one of our goals, just not the most important goal

Regardless of my code examples landing, the burden of proof should be on this untested design, not on the proponents of a design that's stood for years. I'm not too concerned with what one person finds "worth considering", I'm more concerned that this spec doesn't make a massive mistake.

@benjamingr
Copy link

@jhusain

Does it concern you that native Promises are sufficientl6y slow that third-party libraries like Bluebird have to replace them entirely?

It does very much. That's why I use bluebird in most of my code and am a contributor on that project. Action is being taken though, Domenic has been working to push this since April and native promise performance will improve significantly - in areas where it can't for spec reasons - "private symbols" will be used to bypass it to allow the exposed APIs to be fast. So overall - it's a problem we're solving, but it's fixable.

The reason native promises are slow is only 10% the spec (and that's being worked on from the "private symbols" angle). The reason promises are slow is the same reason most ES5+ features are slow on engines (and v8 in particular) - there is little incentive to optimize them compared to other efforts.

Don't browsers already have ample incentive to optimize bind?

Well, bind being slower than a closure is only part the implementers fault some stuff about it is harder to optimize. It's definitely optimizable for the vast majority of use cases but to your question: no, they don't really have strong incentive to optimize it given other things they can work on apparently.

Do you perceive that the :: operator (which I'm a big fan of) will make it easier to optimize bind, or merely provide additional incentive by spreading the pattern?

I'm also a huge fan of :: (my only regret is that protocols aren't going in). I think :: is easier to optimize in easy cases (when it's an actual function call) but we have no reason to believe it will be done fast. Implementers tend to do what's fast to ship when they can get away with it - for example: const is a double pointer lookup in v8 at the moment. I seriously hope we get fast :: in JavaScript, I think value objects, async iterators, observables and :: will fundamentally make JavaScript a better language.

Would you agree that it might be valuable to get feedback from browser implementers

Yes, very. Difficulty of implementation is meaningful, especially difficulty of efficient implementation. That said in the case of observables I don't think it should be a problem and in the case of :: I think this is a lot of motivation for :: in the first place.

@benjamingr Convinced you've gotten rid of the closure. Seems like attaching a property to a function might cause deoptimization...

Here goes n+2, I was just addressing @Blesh 's challenge of not making a closure. I did not suggest we actually use this technique in order to do closure elimination. It might be fast in some scenarios, I don't think we're even close to those in RxJS for the most part. You're free to explore it :)

The whole point I was trying to make is that performance shouldn't bother us now (in es-observable) just yet in function vs object. I think this proposal needs some stuff a lot sooner like a modeling to EventTarget, WebIDL, concrete candidate DOM APIs for usage with, consensus in WHATWG, lots of example code in scenarios, sync unsubscribe semantics and a bunch of other stuff.

I think any discussion of performance would be really useful if we involve implementers like you suggested. Without them it's going to be hard to reach meaningful conclusions.

@Blesh

Except that named function created in the external scope. I'm sorry, I don't consider you expert enough to just take your word for it.

There is no closure there. I'm not sure how else I can say it. You don't have to agree with me - lucky for the both of us the language has a specification that confirms it. Before we go there - let me try one more time:

// Hi, I'm a FunctionDeclaration, I create a binding on the environment record
function foo() {}; 
// here, a FunctionExpression, the variable declaration creates a binding on the environment record
var bar = function(){};  // note that the function *does not* create one itself
var f1 = function f2(){}; // f2 does _not_ create a binding in the environment record, f1 does
foo; // function
bar; // function
f1; // function
f2; // ReferenceError, f2 is not a part of the environment here

Basically, the difference between function declarations, function expressions and named function expressions. As I said, you can replace it with arguments.callee if you'd like and get the same result, or use an arrow function and use this directly for that matter.

The environments we need this to be fast in for Netflix are unlucky anything you're going to run into at TipRanks.

Of course, and I completely agree you need to optimize RxJS for a lot more devices and a lot more aggressively than us, no argument there.

I assure you, there is sound reasoning behind why we're interested in a faster RxJS.

And to be clear, I'd like to encourage you to make a faster RxJS, I was pointing out that I find RxJS's performance satisfactory for what I use it for. Go ahead and make it fast, again, I'm rooting for you.

There will be code examples forthcoming, in the form of an altered branch of RxJS Next.

That would be great. Thanks.

if this design affects performance in RxJS Next, we can't pursue it in that library

If this design affects the performance of RxJS in a way that can't be fixed by more precise programming then I agree. I just don't see how that's the case here in this specific example. Those code example would go a long way :)

Regardless of my code examples landing, the burden of proof should be on this untested design, not on the proponents of a design that's stood for years. I'm not too concerned with what one person finds "worth considering", I'm more concerned that this spec doesn't make a massive mistake.

I completely agree with the last point and it's definitely important. I think it's a much more compelling argument than performance at this point. The APIs are very similar and the only difference is that in one they're bound to an object and in the other they're not.

@benlesh
Copy link
Author

benlesh commented Jul 12, 2015

There is no closure there. I'm not sure how else I can say it. You don't have to agree with me - lucky for the both of us the language has a specification that confirms it.

You know what? You're exactly right. For some reason I was looking at/testing function declarations not named function expressions. I'm still interested to see how it shakes out, perf-wise. I'll probably have to use a technique like this to eliminate closure.

@jhusain
Copy link
Collaborator

jhusain commented Jul 12, 2015

@benjamingr Great to hear about the work happening in the Promise space. Feeling very encouraged that Observables can benefit from this work. I'm also more hopeful that the current spec can be made to be fast with these optimizations. Still interested to get more context on the problem of bind optimization from an implementer, maybe @mulambda?

Obviously Netflix will have to do what it has to to meet its performance requirements - even if that means diverging from spec. Holding out hope that either the fast path or closure elimination technique makes this unnecessary. Looking forward to seeing some code.

@benlesh
Copy link
Author

benlesh commented Jul 13, 2015

@zenparsing @jhusain... I realize Subject is not part of this proposal, but if there are no Observers, how can we subscribe to Observable with a Subject? How can we even have a Subject?

@benjamingr
Copy link

Since you don't have subjects you don't subscribe to observables with a
subject. You only subscribe to observables with callbacks with the callback
design.

On Mon, Jul 13, 2015 at 4:17 PM, Ben Lesh notifications@github.com wrote:

@zenparsing https://github.com/zenparsing @jhusain
https://github.com/jhusain... I realize Subject is not part of this
proposal, but if there are no Observers, how can we subscribe to Observable
with a Subject? How can we even have a Subject?


Reply to this email directly or view it on GitHub
#41 (comment)
.

@jhusain
Copy link
Collaborator

jhusain commented Jul 13, 2015

I didn't really think the observer had a lot of value on its own. However it does appear to be useful as a contract as @Blesh points out.

Composing together Observables and Subjects is a common enough operation that I would like it to be ergonomic. Subjects don't need to be in the specification for us to consider this, they are common enough in UserLand to constitute a use case we should consider.

Even if the bind operator lands, I'm still concerned about ergonomics here:

observable.subscribe(::subject.next, ::subject.throw, ::subject.complete);

With arrow functions its (not tremendously) worse. However it's also very error prone.

observable.subscribe(x =>subject.next(x), e => subject.throw(e), ()=> subject.complete());

I'm concerned that for specification expediency, we are resisting encoding this contract. I'm also concerned that part of the reason is that it looks so similar to a generator.

@zenparsing
Copy link
Member

@Blesh We definitely want subjects to be possible. I think using the callback style you just have to use the "next" callback as your listener identity, and store that along with the other callbacks somehow (in a Map or in an object literal or something).

It's definitely more straightforward in this case to have observers instead of callbacks though.

@jhusain Over the weekend, I started having similar concerns over ergonomics.

I'm concerned that for specification expediency, we are resisting encoding this contract. I'm also concerned that part of the reason is that it looks so similar to a generator.

Can you explain the second sentence?

@benjamingr
Copy link

Hmm, I'm starting to like the original better:

  • we have .forEach for iteration, it takes a callback and returns a promise for completion. Maybe it should be renamed as it doesn't indicate (unlike C#) that it creates a subscription and "fires the action". (Maybe .run or .consume or something like that).
  • we have observers for subscribe, these return subscriptions too. This means subjects (which I also use a lot in RxJS) are easy and a straightforward subtype.

Calling things generators or generator return results was super confusing, it took me at least a day to see @jhusain 's point about duality, it is beautiful but it will be very confusing for new users to see generators as both ends of the pipe. It took me a while to see he was doing this thing, it will likely take average users as long as me.

I'm wondering if we can keep next/done/return as a signature but not claim duality, not say a word about generators and just call the next/done/return signature with no return types "observer". We can then claim a generator return result is both the subtype of a regular iteration result and a subtype of an observer which is nice from a "type system point of view". People who explicitly want to "prime" will be able to, but it won't be advertised at the spec level.

This sounds like the best of all worlds.

It's much closer to the original design.


I think that the return value of forEach and subscribe can be unified into a subscription-promise being that doesn't use promise cancellation but is observable-cancellable. Cancellation can be synchronous (not sure why not anyway) with sound semantics.

This has added benefits like subscribe being awaitable, forEach returning a cancellable entity and so on.

@benlesh
Copy link
Author

benlesh commented Jul 13, 2015

Actually, I'll just start a new issue

@jhusain
Copy link
Collaborator

jhusain commented Jul 13, 2015

Hey Benjamin. Very excited to see you are open to this because I was in the middle of proposing pretty much the same thing. Happy we're closer to alignment on this, and sorry if my explanations of duality were lacking. I'm no @headinthebox clearly. :)

Looking forward to discussion on new thread.

JH

On Jul 13, 2015, at 7:51 AM, Benjamin Gruenbaum notifications@github.com wrote:

Hmm, I'm starting to like the original better:

we have .forEach for iteration, it takes a callback and returns a promise for completion. Maybe it should be renamed as it doesn't indicate (unlike C#) that it creates a subscription and "fires the action". (Maybe .run or .consume or something like that).
we have observers for subscribe, these return subscriptions too. This means subjects (which I also use a lot in RxJS) are easy and a straightforward subtype.
Calling things generators or generator return results was super confusing, it took me at least a day to see @jhusain 's point about duality, it is beautiful but it will be very confusing for new users to see generators as both ends of the pipe. It took me a while to see he was doing this thing, it will likely take average users as long as me.

I'm wondering if we can keep next/done/return as a signature but not claim duality, not say a word about generators and just call the next/done/return signature with no return types "observer". We can then claim a generator return result is both the subtype of a regular iteration result and a subtype of an observer which is nice from a "type system point of view". People who explicitly want to "prime" will be able to, but it won't be advertised at the spec level.

This sounds like the best of all worlds.

It's much closer to the original design.

I think that the return value of forEach and subscribe can be unified into a subscription-promise being that doesn't use promise cancellation but is observable-cancellable. Cancellation can be synchronous (not sure why not anyway) with sound semantics.

This has added benefits like subscribe being awaitable, forEach returning a cancellable entity and so on.


Reply to this email directly or view it on GitHub.

@domenic
Copy link
Member

domenic commented Jul 13, 2015

Bikeshedding: the problem with return is that it would usually bring along the notion of return value, which has problems such as #27. Maybe a no-argument complete would be better.

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

No branches or pull requests

6 participants