fsprojects / FSharp.Control.Reactive

Extensions and wrappers for using Reactive Extensions (Rx) with F#.
http://fsprojects.github.io/FSharp.Control.Reactive
Other
284 stars 58 forks source link

Implement Disposable.compose as pure function, add tests #141

Closed deviousasti closed 4 years ago

deviousasti commented 4 years ago

Fixes #139 and #140

deviousasti commented 4 years ago

Regarding #139, the order chosen is the standard order for List/Seq/Array.append.

List.append [1; 2]  [3; 4] //[1; 2; 3; 4]
mrakgr commented 4 years ago

Wait a bit. You've made things too complicated here.

I was 100% sure before I looked at your code that you would have implemented compose like...

let compose (a : #IDisposable) (b : #IDisposable) = Disposable.Create(fun () -> a.Dispose(); b.Dispose())

There isn't actually that much difference between a explicit list and a function such as this one. In fact, it would not surprise me that your version results in more memory taken up as every list element is an object (which are 24b when empty.) At most the extra allocation in Disposable.Create might be a boolean flag so repeat calls to dispose are ignored.

Consider the situation where you have two IDisposables you want to compose.

In my situation compose would allocate one extra object made by create. In your situation that would be 2 because it has to wrap all the disposables as a list.

Also I am in favor of keeping the original argument order like...

let compose (b : #IDisposable) (a : #IDisposable) = Disposable.Create(fun () -> a.Dispose(); b.Dispose())

This makes it easier to write things like...

a
|> compose b
|> compose c
|> compose d

With the compose (b : #IDisposable) (a : #IDisposable) version the above fragment would get disposed in the a,b,c,d order, the same top-down order it was written. This was one thing the original version got right.

Also if you use your particular order and consider how compose would usually be used, you'll see that appending an element to the end of the list would be the most common use case which is inefficient as it would constantly need to be traversed and recreated.

mrakgr commented 4 years ago

The reason the List.append is the way it is so you can pass it as an argument to List.fold. For example, you can implement List.concat as let concat l = List.fold List.append [] l. It composes well like this. But with IDisposable, you can easily do the above in an efficient manner by simply passing the sequence to ComposableDisposable. Like let compose (l : _ seq) = new CompositeDisposable(l) for example.

deviousasti commented 4 years ago
let compose (a : #IDisposable) (b : #IDisposable) = Disposable.Create(fun () -> a.Dispose(); b.Dispose())

Interestingly, this is exactly how I first implemented compose. However, the call stack grows with each compose, which I suspect was the original motivation for the CompositeDisposable implementation.

In the example tests, you can see it being used as List.reduce Disposable.compose. I have the older version saved - maybe @panesofglass can take a call on this. I can push that version if needed.

mrakgr commented 4 years ago

However, the call stack grows with each compose, which I suspect was the original motivation for the CompositeDisposable implementation.

You mean when calling a.Dispose()? Don't worry about this, it is no different than doing a foldBack over a list. You'd have to make the same traversal when appending to the end of the list like in your current version.

In my version you'd only do it once - when the whole thing gets disposed. In b.Dispose() gets called in tail position so it does not grow the call stack.

deviousasti commented 4 years ago

I’m not sure being in tail position always guarantees a tail-call, It has to happen on the JIT. I ran some tests and it doesn't seem to be optimized. Not sure if it's because dispose is a virtual call.

I do agree on the order though – it reads better for piping.

mrakgr commented 4 years ago

Interesting. Well, the unnecessary allocations the ones you really want to avoid. But the necessary ones, the ones which improve correctness you want to embrace. With compilers there is always the hope that some future version of F# will have better tail call optimization. And the main point I think is that IDisposable allocation is not something you'd necessarily want to optimize over correctness as it is rare.

Interesting tidbit - languages like my own Spiral can inline these Disposable.compose nested functions so that in the end you'd get imperative control flow without any allocation. It depends on factors like whether you are passing or returning them from functions (join points in Spiral's case), returning them from if statements, or putting them into references or arrays.

I don't think that F# or .NET's optimizer is good at this, but my view is that straightforward purely functional code is a lot easier to optimize than imperative one. In Spiral, it is easy to do what would in other languages be metaprogramming simply by paying attention to data flow.

deviousasti commented 4 years ago

The only strong guarantee that we have tail-calls is the presence of the tail opcode in the IL (which is absent here), which the JIT always follows, otherwise it's up to the JIT to decide if it's a valid case for optimization.

Purely functional code definitely has much more potential to be optimized. Just the absence of loops and flow control itself makes rewriting rules much more amenable.

deviousasti commented 4 years ago

Removed it and amended.