4.x: Dedicated Amb implementation for arrays and enumerables #545
Conversation
| var n = 0; | ||
| var sources = new IObservable<T>[8]; | ||
|
|
||
| try |
There was a problem hiding this comment.
Just use ToList or ToArray here.
There was a problem hiding this comment.
This basically avoids the final copy in ToArray to the actual size as the internal expanding array will have slack otherwise. We can simply use n to limit the evaluation of the array here.
There was a problem hiding this comment.
No please, slack is just fine, this is not the place to save bytes, we're possibly re-allocating and copying here more often than needed, the Linq-internal ToArray does the trick just fine with a good resize-strategy.
There was a problem hiding this comment.
Here is that extra copy which is avoided in the PR:
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok, use ToList then. Should do the same without extra copy.
| Volatile.Write(ref winner, -1); | ||
| } | ||
|
|
||
| internal static IDisposable Create(IObserver<T> observer, IObservable<T>[] sources, int n) |
There was a problem hiding this comment.
n is pretty much redundant, right?
| return parent; | ||
| } | ||
|
|
||
| internal void Subscribe(IObservable<T>[] sources, int n) |
There was a problem hiding this comment.
Use observers.Length for n
|
|
||
| public void Dispose() | ||
| { | ||
| var o = observers; |
There was a problem hiding this comment.
Can you not use those intermediate values, observers is readonly so this might be optimized by JIT, and o.Length is constant since o is an array.
There was a problem hiding this comment.
I don't know the exact rules but even if they are readonly fields, it is possible the compiler would re-read the fields around those Volatile accesses all the time. This way, there is practically no chance the compiler inlines the read of the field back into all those places thus causing the aforementioned re-reads.
There was a problem hiding this comment.
I am pretty sure it won't since it's all constant anyway. Anyway, it's just a lot of noise.
There was a problem hiding this comment.
It is part of the optimized logic which also prevents bad decisions on the JIT's part, which I came to not expect much of right now.
There was a problem hiding this comment.
What makes you think the JIT makes bad decisions?
There was a problem hiding this comment.
Looks like I can't convince you about this. I'll update the PR.
There was a problem hiding this comment.
I haven't done a lot of these micro-optimizations, and there might be good reasons to do this, but I have learned not to try to outsmart the JIT, because it usually does a pretty good job, and there is always a tradeoff wrt. readability, which is one of my concerns also. Plus, we want to get a lot of people involved in this, and not everybody can write that kind of code.
| } | ||
| catch (Exception ex) | ||
| { | ||
| observer.OnError(ex); |
There was a problem hiding this comment.
Won't this be handled by SafeObserver?
There was a problem hiding this comment.
I don't think there is a try-catch guarding the IProducer.Run method. It would likely crash out of the Subscribe method and count as a badly implemented IObservable:
There was a problem hiding this comment.
If that's an issue, we should discuss that separately for all operators and put it in a central place because no other operator guards the Run like this.
There was a problem hiding this comment.
This nothing new and is there in operators that call delegates in their Subscribe/Run methods. Nothing to worry about.
There was a problem hiding this comment.
ok, because you're enumerating? Fine for now I guess, but if it's a pattern, it should be....well, a pattern.
This PR adds dedicated, lock-free implementation for
Amb(params IObservable<T>[] sources)andAmb(this IEnumerable<IObservable<T>> sources).The original internal implementation was chaining the two-argument
Ambs which was both wasteful and possibly prone to stack overflow due to the nesting.