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

One single global stateless OnCompleted Notification object #662

Closed
samuelgruetter opened this issue Dec 23, 2013 · 6 comments
Closed

One single global stateless OnCompleted Notification object #662

samuelgruetter opened this issue Dec 23, 2013 · 6 comments

Comments

@samuelgruetter
Copy link
Contributor

Instead of creating a new OnCompleted Notification object every time we need one, we could have one global instance, accessible by a factory method on Notification (suggested by @akarnokd here).

Advantages:

  1. Better performance, since fewer object (=garbage) creation
  2. Scala adaptor could have an object OnCompleted instead of a class OnCompleted which has to wrap a Java OnCompleted object.

Maybe we could also get 2) without any changes in Java, but then we might need to define clearly if the toScalaNotification/toJavaNotification methods have to preserve object identity, or only equals.

@benjchristensen
Copy link
Member

Does this really make much of a difference when the onNext notification is still needed and will overwhelm the number of onCompleted notifications when an Observable emits more than 1 onNext?

In general we probably need to move away from (or be cautious about) the use of materialize and Notification for internal operators as the cost is quite high as shown by performance work done on the ReplaySubject: a144b0e and 8425eff

@samuelgruetter
Copy link
Contributor Author

The reason why I opened this is that I want to make OnCompleted a singleton object in the Scala adaptor. This is possible with the current Java Notifications, but before I do it, I want to check if there will be changes in the Java Notifications, so that I can avoid doing the work twice.

I think there might be changes in the Java Notifications because I see some issues:

  1. Creating a new Notification object for each OnCompleted requires more memory than having one single OnCompleted object.
  2. You can't make Notifications for Observable<Exception>, because new Notification(new Exception()) is ambiguous: Should it be an OnNext or an OnError Notification?
  3. IMO a library should never expose constructors, but always keep them private and provide factory methods, since it gives the library implementors more flexibility (the issue we're discussing here is an example for this).
  4. In the Scala adaptor, we want a single object OnCompleted, because this is exposed to the users when they want to do pattern matching, so it would be nice to have the same structure in Java.

There's a non-breaking way to solve these issues:

  1. As @benjchristensen points out, this won't make much of a difference when the onNext notification is still needed and will overwhelm the number of onCompleted notifications when an Observable emits more than 1 onNext.
  2. Don't make Observables of exceptions, because that confuses everyone.
  3. Disagree with me because I wrote IMO ;-)
  4. It's enough to have toJavaXxx(toScalaXxx(myJavaXxx)).equals(myJavaXxx), we don't need to guarantee that toJavaXxx(toScalaXxx(myJavaXxx)) == myJavaXxx (where == is the Java ==), so it's possible to have a singleton OnCompleted object in Scala and many OnCompleted instances in Java.

And there's also a breaking (but clean and nice) way to solve these issues:

  • Remove (or deprecate) the constructors and replace them by factory methods.

I'm for the second way, but since both ways are fine for the Scala adaptor, I won't insist more than this and let you decide. And once it's decided, I'll make the changes in the Scala adaptor.

@headinthebox
Copy link
Contributor

I agree with Ben, It does not seem to matter much perf-wise to special case Notification.OnCompleted, also since whenever you use Notifications you are already paying a hug interpretative overhead in doing case analysis. But maybe I am missing something?

That said, in Scala Notification.OnCompleted is already using a factory method, so if you really want to use a singleton object you can do that in Scala.

@benjchristensen
Copy link
Member

Just tried to make a static OnCompletedNotification but can't do it as it uses generics and must have a type T as the signature is Notification<T>.

I don't see how to make a static representation even if we want to while having it be type-safe.

@samuelgruetter
Copy link
Contributor Author

To make it work without casting, we would need a Bottom type, such that we can say OnCompletedNotification extends Notification<Bottom>, and a way to mark Notification as covariant. Since Java doesn't have this, we could use one Notification instance which serves as the global OnCompleted instead of creating a separate class for it. This globalOnCompleted would have type Notification<Void> and we would cast it to Notification<T>, but for users, everything would be typesafe.

@akarnokd
Copy link
Member

akarnokd commented May 8, 2014

Done, could you close the issue?

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

4 participants