Lift Performance

[benjchristensen]

Using f.lift() directly instead of subscribe improves ops/second on the included test from 5,907,721 ops/sec to 10,145,486 ops/sec

[cloudbees-pull-request-builder]

RxJava-pull-requests #813 FAILURE
Looks like there's a problem with this pull request

[benjchristensen]

@abersnaze What about the debug hooks depends on subscribe vs f.call behavior?

[akarnokd]

I like it as it reduces the call stack depth and I don't need to step though the hooks and internal implementation checks all the time. But isn't this lack of safeguards dangerous?

[benjchristensen]

I had always intended it to call f.call directly (as in the prototypes), I think it was inadvertent that it became subscribe. As for safety, we never have wrapped internal operators (see https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L7024) so we are not losing safety. The only place the SafeSubscriber wrapper should happen is at the end, when a user provides an Observer or Subscriber. This is actually important we don't do this, otherwise any onError/onCompleted in the middle of a sequence would invoke unsubscribe and we don't want that.

Since lift is now public for placing operators in the chain, perhaps we want to conditionally wrap them, but I tend to think that if someone is adding an operator it should be a considered an "unsafe" thing and that operators need to comply with the Rx contract. If we do choose to wrap, it would be a conditional check similar to the link I posted above. I don't think it's worth the cost though.

The code would become this:

    public <R> Observable<R> lift(final Operator<? extends R, ? super T> lift) {
        return new Observable<R>(new OnSubscribe<R>() {
            @Override
            public void call(Subscriber<? super R> o) {
                if (!isInternalImplementation(o)) {
                    o = new SafeSubscriber<R>(o);
                }
                f.call(hook.onLift(lift).call(o));
            }
        });
    }

And the perf drops from ~10m ops/second to ~9m.

With conditional check:

Run: 10 - 9,113,444 ops/sec 
Run: 11 - 8,603,038 ops/sec 
Run: 12 - 8,891,892 ops/sec 
Run: 13 - 8,548,031 ops/sec 
Run: 14 - 8,928,571 ops/sec 

Without:

Run: 10 - 10,167,354 ops/sec 
Run: 11 - 10,169,422 ops/sec 
Run: 12 - 10,065,222 ops/sec 
Run: 13 - 10,705,950 ops/sec 
Run: 14 - 10,996,019 ops/sec 

[headinthebox]

I personally refer to "f" as "unsafeSubscribe" and agree with Ben it should do nothing except chaining lift. Only at the end you insert checks. If you add an operator, you are responsible to make sure it does not mess things up.

[cloudbees-pull-request-builder]

RxJava-pull-requests #822 FAILURE
Looks like there's a problem with this pull request