New SortAndBind operator

[RolandPheasant]

Introduce a new bind and sort operator which combine the current Bind and Sort operators.

In the old way of doing it, the sort operator had to produce a sorted set every time there was a change, which in simple terms meant the sort operator had to:

1. Calculate sorted changes with index
2. Copy the entire sorted set for every changed

Then the Bind operator had to replay the differences, or if there was a large change set reload the list from the cloned / sorted set.

This created serious performance issues especially for larges collections. So I have created a BindAndSort operator which combines these operations and I can entirely get rid of cloning the sorted set. Also I have made it such that you can now bind to any collection that implements IList<T>

var comparer = SortExpressionComparer<T>.Ascending(p => p.Id).ThenByAscending(p => p.Name);

var oldWay = myCache.Sort(comparer).Bind(var our myList);

var newWay = myCache.SortAndBind(var our myList, comparer);

// for the new operator there are additional options. For example

var options =new SortAndBindOptions
            {
                InitialCapacity = Count,
                UseBinarySearch = true
            };
var newWay = myCache.SortAndBind(var our myList, comparer, options );

The one slight drawback is that the bind and sort would have to be done on the UI thread, but I don't think this is such a big deal with huge perf gains.

Now for the benchmarks. @JakenVeina can you check over the benchmarks just to be sure I have set them up well.

Initial population

Method	Count	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Gen1	Gen2	Allocated	Alloc Ratio
Old	100	28.53 μs	0.567 μs	0.557 μs	1.00	0.00	6.6833	0.3052	-	61.66 KB	1.00
New	100	18.29 μs	0.365 μs	0.449 μs	0.64	0.01	4.2114	0.0305	-	38.98 KB	0.63
Old	1000	409.07 μs	8.092 μs	9.319 μs	1.00	0.00	88.8672	24.9023	-	820.42 KB	1.00
New	1000	329.29 μs	6.349 μs	11.609 μs	0.81	0.04	66.8945	4.3945	-	616.48 KB	0.75
Old	10000	8,744.38 μs	173.471 μs	474.875 μs	1.00	0.00	1328.1250	671.8750	500.0000	10041.87 KB	1.00
New	10000	5,562.98 μs	41.159 μs	32.134 μs	0.64	0.03	851.5625	281.2500	-	7836.35 KB	0.78
Old	50000	54,583.32 μs	1,065.639 μs	1,562.000 μs	1.00	0.00	5300.0000	600.0000	600.0000	54878.87 KB	1.00
New	50000	44,452.76 μs	887.386 μs	1,153.852 μs	0.81	0.04	4666.6667	-	-	43980.84 KB	0.80

For a single change

The optimized version includes using BinarySearch

Method	Count	Mean	Error	StdDev	Median	Ratio	RatioSD	Gen0	Gen1	Gen2	Allocated	Alloc Ratio
Old	10	1,683.7 ns	20.96 ns	17.51 ns	1,686.1 ns	1.00	0.00	0.2785	-	-	2624 B	1.00
OldOptimized	10	800.3 ns	15.65 ns	29.40 ns	796.5 ns	0.48	0.02	0.1965	-	-	1856 B	0.71
New	10	369.2 ns	6.39 ns	5.67 ns	368.1 ns	0.22	0.00	0.0720	-	-	680 B	0.26
NewOptimized	10	556.3 ns	10.85 ns	13.32 ns	554.1 ns	0.33	0.01	0.0858	-	-	808 B	0.31
Old	100	6,090.6 ns	118.70 ns	141.30 ns	6,091.7 ns	1.00	0.00	1.1673	0.0076	-	11048 B	1.00
OldOptimized	100	1,006.4 ns	14.21 ns	11.87 ns	1,006.4 ns	0.16	0.00	0.3796	0.0019	-	3584 B	0.32
New	100	1,712.9 ns	43.85 ns	125.81 ns	1,662.9 ns	0.27	0.01	0.3014	-	-	2840 B	0.26
NewOptimized	100	686.5 ns	13.74 ns	15.28 ns	681.9 ns	0.11	0.00	0.1163	-	-	1096 B	0.10
Old	1000	49,820.9 ns	991.81 ns	2,506.43 ns	49,273.1 ns	1.00	0.00	9.9487	0.5493	-	93992 B	1.00
OldOptimized	1000	2,211.8 ns	42.17 ns	43.31 ns	2,205.9 ns	0.04	0.00	1.9379	0.1907	-	18272 B	0.19
New	1000	13,712.9 ns	269.31 ns	505.83 ns	13,669.8 ns	0.28	0.02	2.5940	-	-	24440 B	0.26
NewOptimized	1000	907.8 ns	17.21 ns	16.90 ns	904.3 ns	0.02	0.00	0.1469	-	-	1384 B	0.01
Old	10000	731,549.7 ns	12,973.21 ns	22,378.14 ns	721,852.1 ns	1.000	0.00	123.0469	45.8984	42.9688	922226 B	1.000
OldOptimized	10000	24,563.5 ns	2,019.32 ns	5,212.52 ns	24,027.3 ns	0.033	0.01	12.6953	12.6953	12.6953	162677 B	0.176
New	10000	157,586.0 ns	3,135.62 ns	5,889.45 ns	157,696.9 ns	0.216	0.01	25.3906	-	-	239657 B	0.260
NewOptimized	10000	2,072.1 ns	40.10 ns	50.72 ns	2,066.4 ns	0.003	0.00	0.1869	-	-	1768 B	0.002
Old	50000	3,538,953.8 ns	70,395.87 ns	107,501.93 ns	3,549,659.6 ns	1.000	0.00	449.2188	62.5000	46.8750	4602400 B	1.000
OldOptimized	50000	335,438.2 ns	26,403.01 ns	77,849.87 ns	323,205.6 ns	0.122	0.01	17.5781	17.5781	17.5781	802897 B	0.174
New	50000	880,881.6 ns	17,536.51 ns	36,215.94 ns	875,918.3 ns	0.250	0.01	127.9297	0.9766	-	1211196 B	0.263
NewOptimized	50000	7,932.2 ns	152.25 ns	304.07 ns	7,918.4 ns	0.002	0.00	0.1984	0.0153	-	1961 B	0.000

[RolandPheasant]

TODO: I need to add a test case for refresh

[JakenVeina]

BindAndSort_Initial to match BindAndSort_Change?

[JakenVeina]

Class name doesn't match file name.

[JakenVeina]

The proper assertion here, in my mind, is...

_bountList.Should().BeEquivalentTo(peopoe.OrderBy(p => p, _comparer), options => options.WithStrictOrdering());

[RolandPheasant]

You're right.

I cloned there test cases from the original sorted fixture and adapted accordingly. This also meant a very very old hang up from the days that I used NUnit.Assert which did the expectation and actual backwards to the usual conventions. I'll fix this throughout the class.

[JakenVeina]

The proper assertion here, in my mind, is the same as above: just assert the bound list is the source items, sorted.

[RolandPheasant]

I think this is fine to check that a known item has been inserted exactly where we expect it.

With regard to asserting the bound list to be the same as the source.Items sorted, that too would have some merit but I don't think it completely replaces the need for an exact assertion.

[JakenVeina]

Technically, if you're asserting that the whole list exactly matches the desired sorting, that should mean that asserting a single item's position is superfluous, but I'm also a big fan of clarified intent, and I'm not gonna knock a tiny inefficiency in testing.

[RolandPheasant]

I like to combine both. The tests should also not force a cognitive load for the person running or debugging them. Where possible a declarative assertion tells fresh eyes / a newbie exactly what is happening and not just to assert correctness.

[JakenVeina]

Nitpick: InsertInMiddle?

[JakenVeina]

Probably, this is a meaningless micro-optimization, but what I would do is either separate this logic into different subclasses, or make this a Func<TObject, int> field that gets assigned to one method or the other, upon subscription. That way, the if check here isn't getting repeated for every inserted item, it only gets done once, since the options don't ever change.

[RolandPheasant]

I think it's probably a micro-optimization too far as a simple bool check is inexpensive.

[JakenVeina]

This definitely should not be implemented with a constructor, but just plain get/init properties.

A) The goal of any of these Options types should be to contain all the "optional" options for an operator, ergo, a caller should be able to construct a set of options with only the options they care about. A constructor like this forces the caller to specify every single one. Plain object-initializer syntax would not.

B) If we want to add new options in the future, changing the constructor would be a breaking change, just adding a property to the struct is not.

[RolandPheasant]

You're right. I just have not got into the habit of using init properties on records.

It will be in my next push.

[JakenVeina]

I would very much like us to move away from using non-obvious "special values". E.G. from context in the operator, it looks like you're using 0 as the value for ResetThreshold to mean "don't ever convert updates to resets". I think it's way clearer to just use null for that scenario. Similarly, null for InitialCapacity instead of -1, when the intention is to not set an initial capacity.

With those changes, I would say that a Default field here becomes superfluous, as all of the default values for these options then become the default value for the type itself. Unless you want to make the Default field non-readonly, so consumers can set their own defaults, as they see fit.

[RolandPheasant]

Removed Default, but included a system wide one which consumers can configure

[JakenVeina]

I would have comparer on all of these overloads be rolled into BindAndSortOptions, with the default being null and resolved to Comparer<T>.Default within the operator.

[RolandPheasant]

Surely that would throw if the target object does not implement IComparable. I think this would confuse most people and end up with a load of support issues being raised.

[JakenVeina]

It would indeed throw, yes. But it follows the pattern established by Microsoft in System namespaces. List<T>.Sort(), Array.Sort(), and IEnumerable<T>.OrderBy() all support being called without an IComparer<T> being given. If Microsoft thinks it's a reasonable compromise, that's good enough for me.

On a separate note, there's probably also room for an overload that takes a Comparison<T> , instead of an IComparer<T> which is also something all of the above system APIs have.

[RolandPheasant]

Let's do that for Dynamic Data 2. I do not mind making various changes for this version, but I think a change of concepts and limitations is part of the re-design

[JakenVeina]

Are we cool with having some operators be organized like this, with a partial class, and some not? Would it be better to just reorganize ALL of them in one big PR?

[RolandPheasant]

I have been meaning to split these up for years but I have always been way too daunted to tackle in one hit. So let's leave this here as a placeholder. The we can either move gradually, or if anyone is brave enough to do it in one hit.

[JakenVeina]

I'm down either way, just want to be clear. I'll go ahead and plan to do a big PR to reorganize this, once my current plate is clear. It should be a really simple PR, just.... very large... and tedious.

[JakenVeina]

Now for the benchmarks. @JakenVeina can you check over the benchmarks just to be sure I have set them up well.

They look solid to me. I questioned, at first, the idea of reusing the same subscriptions/bindings over and over, in the Change benchmark, but since the change you're doing is a single randomized replace, that shouldn't have any kind of compounding error effect. It becomes an effective test of the cost of the binary/linear search for insertions. The Initial benchmark then covers the cost of a full re-sort of all the items.

[JakenVeina]

I think it makes more sense to consider this a "terminating" operator, I.E. have it perform a subscription and return IDisposable.

The big code smell in my mind, with leaving subscription management up to the downstream consumer, is that the operator will break if the consumer makes multiple downstream subscription. The operator will end up running for each subscription and trying to apply every changeset to the target list multiple times.

If a consumer NEEDS to be able to listen to downstream changes, I think that really necessitates that they split .BindAndSort() into separate .Sort() and .Bind() operators, so that they can share and publish the .Sort() downstream, for both .Bind() and whatever else they need.

[RolandPheasant]

In the very earliest version of Dynamic Data I implemented terminating operators. But they are problematic because:

1. I often end up adding stuff after Bind() in real world apps.
2. If we were to force users to use Publish(), then it's more code for everyone without any real benefit.
3. It's better to let the consumer decide what is terminating.

Besides once SortAndBind has settled I will make Sort obsolete (for cache). Sort is a terrible operator and I have always been somewhat ashamed of how bad it's performance is and dearly want to get rid of if. My proposal is to complete SortAndBind asap, then in the next major release mark Sort as obsolete then remove it entirely in the following major release.

[JakenVeina]

Standard practice is 2 major releases before removing obsolete APIs, from what I've read.

1. I'll defer to you, on that point, since I haven't built too many real-world apps with DD specifically. It still seems really weird to me.
2. I think extra clarity in API surface is a "real benefit", but how much is definitely subjective.
3. I see it as the consumer having to do extra work, either way, it's just that one way is potentially dangerous, and the other is just slightly more tedious. Like, the consumer doesn't get to "decide" whether .Bind() is terminating it ALWAYS is, if you don't ensure only one subscription, it breaks. Would it be overkill to have it be terminating by returning a custom subscription object, instead of just IDisposable? Something like...

public class Binding<TObject, TKey>
    : IObservable<IChangeSet<TObject, TKey>>,
        IDisposable
{ }

This solves the problem of preventing multiple subscriptions, but still allows additional downstream listeners.

Also, I would kinda like a chance to see if we (or just I) can rework .Sort() for caches into something reasonable, before comitting to dropping it. It seems far too limiting to me to say "you can only sort as the very last step", surely there's a way to make it not terrible. Maybe we can workshop it in DD2 and then pull what works into DD.

[RolandPheasant]

Regarding terminating operators, surely it is something to be considered for Dynamic Data 2 and not the current version.

For context I have historically done stuff like this:

   mySource
      .Sort(...)
      .Bind(....)
      .Subscribe(_> 'Do some ui related action which requires the bound list to be updated first' )

My preference is not to have them but I am willing to be consider it - you make a good point about subscribing twice, but in practice I don't think anyone has ever complained about that being an issue.

Honestly, do not bother with Sort. I spent more time on it, and had more trouble than on any other operator. Also the sorted changeset is fatally flawed as it need to maintain a copy of sorted state after ever change, which is transmitted on the sorted change set. This leads to huge memory allocations and the CPU goes crazy, and for what? The only operators which use sort are Bind, Virtualise and Page. Without sort it would mean we could also get rid of this pesky sorted change set. Also without sort in the cache we do not need index on the changes.

[RolandPheasant]

... for list however, sort is perfectly legit.

[JakenVeina]

Slight nitpick: I think SortAndBind() makes more sense, since that's logically what the operator is doing, sorting first, then binding. Put differently, what this operator replaces is .Sort().Bind().

[RolandPheasant]

Agreed

[RolandPheasant]

@JakenVeina I have responded to most of the feedback and the majority of what you say I agree with, and have applied the changes to the code.

Also I added the binary sort option as an extra measure for single change benchmarks. The results clearly show that it should be used whenever possible as it makes a significant difference. However for a small changeset, it is actually slower, so perhaps I should work out the sweet spot and optimize for this case. I propose doing that in another PR

[dwcullop]

Some comments and some questions (which might result in more comments).

[dwcullop]

Is there a reason to NOT use binary search? Maybe it should be the default.

[RolandPheasant]

If the comparer is an impure function, exceptions are throw , or worse the item ends up in the wrong position.

I've fallen for that issue, and if I do I know a huge number of users would so so too.

[dwcullop]

So ResetThreshold == 0 means to never trigger the logic that just redoes the whole list? Which means it will always just apply the individual changes?

It seems like whether or not to update the current or just throw it out and redo it is going to depend on the size of the current list and the number of changes being applied. If the current list has 5 items and you get a changeset of 50 items, then it will make sense to do the Reset. However, if the current list has 1000 items and you get a changeset of 50 items, it might make more sense to do the individual updates.

Maybe I am wrong about this... Maybe if there is 50 items being added to 1000, then it always makes sense to do the Reset. My point is that maybe size of the changeset isn't enough to make a good decision and we should test that to make sure. If it isn't, then maybe this can be a ratio or something.

[RolandPheasant]

Its been that way since day one, and very few people have ever even noticed that behaviour exists so I suggest why change a proven formula.

However, that does not mean that we can't look into these extra options as a later improvement. Personally I do not want that do extra at this stage because

1. I need to apply the same treatment to visualise and to page as they are the only other operators that rely on sort
2. The improvements are already missive and solved a serious problem which DD currently has.

I suggest one this is merged, you can raise an issue to make these suggestions

[dwcullop]

I don't think you need this because the Rx rules should ensure only one event at a time. However, if you want to keep it anyway, since it is only locked in one place, you can just use:
.Synchronize() and skip the extra explicit allocation (there will still be an implicit one).

[RolandPheasant]

I theory no, but I am sure I have seen issues before with the order that changes arrived on sort after an observe on. That however is a little hazy is my memory and I can't remember the exact cause.

The easy test would be to plug it into dynamic trader. That soon highlights threading issues especially if its speed is removed up

[JakenVeina]

I'm with Darrin, if not synchronizing here causes a problem, that indicates a fault in the upstream operator. It's not something we should have to deal with here, even with ObserveOn and SubscribeOn in the mix. Testing this with the dynamic trader app sounds like a good idea.

[RolandPheasant]

Removed. Let's hammer it in Dynamic Trader

[dwcullop]

Should this use GetCurrentPosition so that it can benefit from the BinarySearch when enabled?

[RolandPheasant]

No because refresh implicitly implies we have an immutable object

[JakenVeina]

"mutable object" I think you mean.

[RolandPheasant]

"mutable object" I think you mean.

Yep a typo. I was typing that on my phone at night in the country side, at a climbing crag, in between climbs (I am a boulderer) using my head torch, and without my glasses on 😃

[dwcullop]

This might benefit a lot of cases to avoid copying all the items left to remove and then copying them right back to insert. It is at the the expense of an extra check, but I think it is a good trade-off.

                        var currentIndex = GetCurrentPosition(change.Previous.Value);
                        var index = GetInsertPosition(item);
                        
                        if (currentIndex != index)
                        { 
                            target.RemoveAt(currentIndex);
                            target.Insert(index, item);
                        }
                        else
                        { 
                            target[currentIndex] = item;
                        }

[RolandPheasant]

Great point, I meant to account for this case but forgot. However with anything to do with binding and sorting it is seriously complicated and there's a heap of hidden gotchas!

Some control suites and platforms do not support replace, whiles others do. Remove and insert was the original behaviour in Dynamic Data and I later added the opt in option to use replace instead. I'll include this change in this PR.

[RolandPheasant]

Done

[dwcullop]

Do we need a historic setting on a new operator? Or are you trying to make it easy for people who relied on the old behavior to migrate?

[RolandPheasant]

In most cases you want a reset. Some user controls out there do not support reset, or maybe there's a requirement to notify individual item changes after bind. Only then is there a need to opt in.

[RolandPheasant]

I thought about this one, and have gone with you suggestion. Done and checked in.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.