[RFC] Add the "subscribeImmediately" parameter to the ObservableAsPropertyHelper class

[flagbug]

When this parameter is set to true, the ObservableAsPropertyHelper
immediately subscribes to the source sequence.

[flagbug]

@kentcb We can add this line, even if we don't merge this PR, so connect isn't called a second time in Value

[kentcb]

I know it's a big, scary breaking change and all, but my inclination would be to default subscribeImmediately to true (or - even better - rename the parameter to delaySubscription or deferSubscription and default it to false). That way, people aren't trolled by the default, and if/when they discover performance issues with OAPH they can dig deeper and move away from the default behavior. The pit of success with OAPH is way too narrow atm. This PR will allow people to widen the pit, but I'd rather just have the pit wider by default.

[moswald]

I would agree with that. deferSubscription would be my choice, since "delay" indicates you're waiting on some sort of timespan to pass.

[flagbug]

The reason I think that delaying the subscription by default is good, is because I think that the constructor of an object shouldn't fire off any work on other threads.

Take the following class for example:

public class MyViewModel : ReactiveObject
{
    private readonly ObservableAsPropertyHelper<int> myProperty;

    public MyViewModel()
    {
        this.myProperty = CalculateAsync().ToProperty(this, x => x.MyProperty);
    }

    public int MyProperty => this.myProperty.Value;

    public IObservable<int> CalculateAsync()
    {
        // Calculate something expensive here
    }
}

If myProperty would subscribe to the source immediately, the construction of MyViewModel now fire of some potentially expensive work on another thread, while the current behavior would subscribe when accessing MyProperty the first time

[kentcb]

Isn't this a case of catering the esoteric scenario at the expense of the common?

I don't think it's a stretch to say that most OAPH usage will simply be massaging existing properties into new ones. And to have those scenarios constantly require that people remember to specify deferSubscription = false seems backwards to me. After all, the people who are kicking off expensive work in a ctor are likely to notice and immediately remember that they should defer that particular subscription (or, if it's the first time they've come up against the problem, look into OAPH a bit and find the solution).

[flagbug]

Yeah, you're probably right that my use case isn't very common.

Maybe @paulcbetts also has some input on this?

[shiftkey]

cc @niik for feels on this change

[kentcb]

Doesn't seem to be any further feedback on this and I would ❤️ for this to get into the 7 branch ASAP so people can start banging on it.

@flagbug could you please change the argument name to deferSubscription and default it to false?

[flagbug]

@kentcb Sure! I'll open a new PR for that change, because I deleted my fork where this PR is based 🙈

[shiftkey]

Just did a quick scan of GitHub Desktop's code - we have 66 .ToProperty() methods, and only 10 of those set scheduler: Scheduler.Immediate to address this issue. I can look into why we've done that where we have, but I still favour lazy for our scenarios - even when in tests.

Not sure how flipping the default behaviour here from lazy to eager will impact us, but I suspect it will. I don't want to hold this up this PR from being merged (I like this being exposed directly, and it's more clearer than using Scheduler.Immediate), but I'm curious about whether this is the right defaults (because I'm a fan of being lazy in general when programming).

[kentcb]

Isn't scheduling a different issue entirely? The purpose of the deferSubscription parameter is to facilitate a lazy subscription, whereas scheduling just dictates the context in which the target property will be updated. Scheduling is of no consequence if the property is lazy and nothing ever connects to it.

[kentcb]

@shiftkey also, allow me to flip the question around a little: what negative consequences could there be from making the default behavior a hot observable? To me, this aligns the feature far more with people's expectations. We've seen that time and again through support questions and (certainly for me anyway) our own experiences using OAPH.

The only possible argument I see against making it hot is performance, and I would consider that a poor argument because:

1. If you really want lazy, you can still get it simply by passing deferSubscription: true
2. Why are people defining these properties if nothing is actually observing it? Surely that's the exception rather than the norm.
3. Performance at the expense of usability is a poor trade-off, imo, especially when that performance can be reinstated per (1)

[shiftkey]

@kentcb I shall check the history on these changes to understand why we've done the Scheduler.Immediate thing. I believe this is the same issue, but perhaps viewed from two different perspectives...

[shiftkey]

@kentcb

To me, this aligns the feature far more with people's expectations. We've seen that time and again through support questions and (certainly for me anyway) our own experiences using OAPH.

I'm probably missing some anecdotes on this, as aside from this Scheduler.Immediate thing our usage of OAPH has been largely conventional. Do you have a good example of newbies running into this so I can get some extra context?

If you really want lazy, you can still get it simply by passing deferSubscription: true

This is true - and I'm not afraid of writing our own extension methods over this to ensure we do it by default.

Why are people defining these properties if nothing is actually observing it? Surely that's the exception rather than the norm.

Perhaps this is the issue at hand. Something definitely necessitated us doing this, which made us feel bad at the time. I'll let you know what I find out.

Performance at the expense of usability is a poor trade-off, imo, especially when that performance can be reinstated per (1)

For most of the usages as I see it we're not talking about expensive operations. So it's gotta be something else.

[flagbug]

One common example that just came to my mind is the Android RecyclerView. RecyclerView's list items are virtualized, so only items that come into the view are actually used. This means that with deferred OAPH subscription, most of the ViewModel properties are never subscribed to, because the ViewModel never actually comes into view.

With the new behavior of immediate subscription, all the properties are loaded at once, as soon as the ViewModels for the RecyclerView are created, which can potentially bog down performance with a lot of items (e.g if the ViewModels also load images)

[kentcb]

@flagbug: interesting. My approach to such a scenario would be to only load the image if the VM is activated.

Here are some other examples:

• testing. Often I've run into issues testing because an assertion is being made against a property that is using OAPH. Of course, if nothing is subscribed to it then the test will fail, but it's not always simple to track down
• non-binding scenarios. For example, a VM exposes a property A that the view binds to. It derives a property B from A using OAPH (to do some further munging of the value). It wants to pass the value of B into the service layer whenever a command executes. But nothing is subscribed to B, so the value is unset.
• non-RxUI binding scenarios. For example, your VM uses OAPH and your iOS version uses RxUI binding. All is well. Then you do a WPF port (or whatever) and for perf reasons you have to use native bindings for a particular view. Things don't work because there are no subscriptions.

[flagbug]

I've created the new PR at #1061

@kentcb Don't get me wrong, I don't like the current behavior, especially the switch where in unit tests, OAPH immediately subscribes to the source, which makes unit tests behave different than production code.

I just wanted to show a case where the current behavior is beneficial, if you have a VM with 10 properties times a list with 100 items, that's at least 1000 calls to Subscribe, which is suuuuuuuuper slow, especially on Android. So I guess what I want to say is that we have to make sure that people are aware of this and aren't blaming RxUI on the slowness, since they now can use the deferSubscription parameter

[flagbug]

Closing in favor of #1061