New Virtualization operator #888

RolandPheasant · 2024-03-28T09:59:44Z

Introduce SortAndVirtualize which replaces the old Vitualise mechanism. This is part of the drive to eliminate the Sort operator altogether.

Old

var virtualRequests =new BehaviorSubject<IVirtualRequest>(new VirtualRequest(0, 25))

myCache 
  .Sort(myComparer)
  .Virtualise(virtualRequests)

New

var virtualRequests =new BehaviorSubject<IVirtualRequest>(new VirtualRequest(0, 25))

myCache 
  .SortAndVirtualize(myComparer)

Additionally I also included a variant of SortAndBind for binding to the virtual output. This works on IList<T> and ReadOnlyObservableCollection<T>

var virtualRequests =new BehaviorSubject<IVirtualRequest>(new VirtualRequest(0, 25))
var myList = new List<MyType>();

myCache 
  .SortAndVirtualize(myComparer)
  .SortAndBind(myList)  \\ comparer is extracted from the output of virtualize

Once this is in, it's almost a direct lift of this code for Page. Then we can mark Sort as obsolete, and at some point delete the old sort + remove the index and previous index from the change object.

…rtualize

RolandPheasant · 2024-03-28T10:00:37Z

src/DynamicData/Cache/Internal/UniquenessEnforcer.cs

-*
+/*
+    For refresh,  we need to check whether there was a previous add or update in the batch. If not use refresh,
+    otherwise use the previous update.
 */

        source


For virtualize I had to ensure unique changes, which previously did not work for Refresh

RolandPheasant · 2024-03-28T10:03:08Z

src/DynamicData/Cache/ObservableCacheEx.VirtualiseAndPage.cs

+
+namespace DynamicData;
+
+/// <summary>


I have included Virtualize, Page and Top operators here. Maybe overkill to have one file per operator. Thoughts?

I agree. We don't need one file per operator. We just need each file to not become unmanageably large. Putting several closely related operators together is fine with me.

Me, I would probably stick with one operator per-file, just for clarity.

dwcullop · 2024-03-28T20:33:51Z

src/DynamicData/Cache/ObservableCacheEx.VirtualiseAndPage.cs

+
+namespace DynamicData;
+
+/// <summary>


I agree. We don't need one file per operator. We just need each file to not become unmanageably large. Putting several closely related operators together is fine with me.

dwcullop · 2024-03-28T20:38:15Z

src/DynamicData/Cache/ObservableCacheEx.VirtualiseAndPage.cs

+    /// <param name="virtualRequests">The virtualizing requests.</param>
+    /// <returns>An observable which will emit virtual change sets.</returns>
+    /// <exception cref="ArgumentNullException">source.</exception>
+    public static IObservable<IChangeSet<TObject, TKey, VirtualContext<TObject>>> SortAndVirtualize<TObject, TKey>(this IObservable<IChangeSet<TObject, TKey>> source,


This thought applies to SortAndBind as well, but I know that's already published...

Do we really need to add a completely new operator here? Couldn't it just be an overload of Virtualize that has an extra IComparer<TObject> (or IObservable<IComparer<TObject>>) parameter? It might make it easier to discover.

I am torn on this one. Adding the SortAnd... clearly marks it apart from the old way. However, you're absolutely correct that it may not be required at all. That also holds true for the SortAndBind operator. Do we just rename these back to the original versions without the SortAnd prefix? I think we probably need a face to face on this question

Using just .Virtualize() and .Bind() only works if there's no ambiguity at the call site.

using var subscription = someStream .SortAndBind(out var target) .Subscribe();

Right now, this is a valid invocation of the operator, if the items in someStream are IComparable<TObject>. If we were to swap .SortAndBind() to just .Bind() we lose a fair bit of clarity, I think, that there's a sort happening. And what if the user wants to opt-out of that implicit sort? Is that even possible?

I'm not sure what the equivalent scenario would be for virtualization, but that's the perspective we want to look at, to consider .SortAndVirtualize() versus just .Virtualize().

dwcullop · 2024-03-28T20:51:56Z

src/DynamicData/Cache/Internal/UniquenessEnforcer.cs

+                        var last = c.Last();
+
+                        // If the last is a refresh we need to check whether, there was a previous add or update.
+                        // If so , use the previous.
+                        if (last.Reason != ChangeReason.Refresh)
+                            return last;


With the logic you have below, I don't think you need this part. It would make sense as an optimization of a very common case, but I don't think that it would be faster than just letting the other logic run.

We need a benchmark here. Maybe a future improvement

dwcullop · 2024-03-28T20:58:15Z

src/DynamicData/Cache/Internal/UniquenessEnforcer.cs

+                        if (all.Length == 1)
+                            return last;
+
+                        /* Extreme edge case where compound has mixture of changes ending in refresh */
+
+                        // find the previous non-refresh and return if found
+                        for (var i = all.Length - 1; i >= 0; i--)
+                        {
+                            var candidate = all[i];
+                            if (candidate.Reason != ChangeReason.Refresh)
+                                return candidate;
+                        }
+
+                        // the entire batch are all  refresh events
+                        return all[0];


Consider:

var all = c.ToArray(); if (all.Length > 1) { /* Extreme edge case where compound has mixture of changes ending in refresh */ // find the previous non-refresh and return if found for (var i = all.Length - 1; i >= 0; i--) { var candidate = all[i]; if (candidate.Reason != ChangeReason.Refresh) return candidate; } } // the entire batch are all refresh events return all[0];

Point being... If you're writing manual loops to avoid LINQ performance penalties, then I would avoid using Last as well.

Actually it's less that I am using linq to avoid performance pitfalls, it more that I need to track backwards from the end of the collection. I also suspect you're right about avoiding using last but my rationale is that for 99% of all cases there will only ever be a single item in the grouping ... so I reasoned that avoiding a to list / array was possibly the best, and that .last() would only iterate once is almost all cases.

It's a tough balance to make, and it would be interesting to get some perf stats on it.

...ignore me, I already do a to array. I should revisit that code!

I've changed the code as per your suggestion.

dwcullop · 2024-03-28T21:00:45Z

src/DynamicData/Cache/ChangeSet.cs

+/// <typeparam name="TObject">The type of the object.</typeparam>
+/// <typeparam name="TKey">The type of the key.</typeparam>
+/// <typeparam name="TContext">The additional context.</typeparam>
+public sealed class ChangeSet<TObject, TKey, TContext> : ChangeSet<TObject, TKey>, IChangeSet<TObject, TKey, TContext>


I'm going to have to keep in mind this is a thing now. I can't quite place it, but I feel like I needed it before...

Yes, I needed some extra params to be passed down the chain, and historically I have inherited and created a new changeset. However I think this is an improvement.

I'm going to have to keep in mind this is a thing now. I can't quite place it, but I feel like I needed it before...

I'll wager it was for one of your various stream merging scenarios.

JakenVeina · 2024-04-01T23:23:05Z

src/DynamicData.Tests/DynamicData.Tests.csproj

+  <ItemGroup>
+    <Compile Remove="Cache\SortAndBindVirtualize.Fixture.cs" />
+  </ItemGroup>


Leftover development noise?

Yes, removed.

JakenVeina · 2024-04-01T23:26:47Z

src/DynamicData/Cache/ChangeSet.cs

+/// <typeparam name="TObject">The type of the object.</typeparam>
+/// <typeparam name="TKey">The type of the key.</typeparam>
+/// <typeparam name="TContext">The additional context.</typeparam>
+public sealed class ChangeSet<TObject, TKey, TContext> : ChangeSet<TObject, TKey>, IChangeSet<TObject, TKey, TContext>


I'm going to have to keep in mind this is a thing now. I can't quite place it, but I feel like I needed it before...

I'll wager it was for one of your various stream merging scenarios.

JakenVeina · 2024-04-01T23:31:12Z

src/DynamicData/Cache/ObservableCacheEx.VirtualiseAndPage.cs

+
+namespace DynamicData;
+
+/// <summary>


Me, I would probably stick with one operator per-file, just for clarity.

JakenVeina · 2024-04-01T23:41:58Z

src/DynamicData/Cache/ObservableCacheEx.VirtualiseAndPage.cs

+    /// <param name="virtualRequests">The virtualizing requests.</param>
+    /// <returns>An observable which will emit virtual change sets.</returns>
+    /// <exception cref="ArgumentNullException">source.</exception>
+    public static IObservable<IChangeSet<TObject, TKey, VirtualContext<TObject>>> SortAndVirtualize<TObject, TKey>(this IObservable<IChangeSet<TObject, TKey>> source,


Using just .Virtualize() and .Bind() only works if there's no ambiguity at the call site.

using var subscription = someStream .SortAndBind(out var target) .Subscribe();

Right now, this is a valid invocation of the operator, if the items in someStream are IComparable<TObject>. If we were to swap .SortAndBind() to just .Bind() we lose a fair bit of clarity, I think, that there's a sort happening. And what if the user wants to opt-out of that implicit sort? Is that even possible?

I'm not sure what the equivalent scenario would be for virtualization, but that's the perspective we want to look at, to consider .SortAndVirtualize() versus just .Virtualize().

JakenVeina · 2024-04-01T23:44:20Z

src/DynamicData/Cache/ObservableCacheEx.cs

-    /// <summary>
-    /// Returns the page as specified by the pageRequests observable.
-    /// </summary>
-    /// <typeparam name="TObject">The type of the object.</typeparam>
-    /// <typeparam name="TKey">The type of the key.</typeparam>
-    /// <param name="source">The source.</param>
-    /// <param name="pageRequests">The page requests.</param>
-    /// <returns>An observable which emits change sets.</returns>
-    public static IObservable<IPagedChangeSet<TObject, TKey>> Page<TObject, TKey>(this IObservable<ISortedChangeSet<TObject, TKey>> source, IObservable<IPageRequest> pageRequests)
-        where TObject : notnull
-        where TKey : notnull
-    {
-        source.ThrowArgumentNullExceptionIfNull(nameof(source));
-        pageRequests.ThrowArgumentNullExceptionIfNull(nameof(pageRequests));
-
-        return new Page<TObject, TKey>(source, pageRequests).Run();
-    }
-


If these operators truly are obsolete, I think we should just mark them as such, and remove them in the future, rather than removing them outright, right now.

SortAndVirtual + SortAndBind - to replace the old Sort followed by vi…

c7e8ebc

…rtualize

RolandPheasant requested review from dwcullop and JakenVeina March 28, 2024 09:59

RolandPheasant commented Mar 28, 2024

View reviewed changes

SortAndVirtual + SortAndBind - fix typos

b4edfed

dwcullop approved these changes Mar 28, 2024

View reviewed changes

Fix build by pleasing the style autocrat

40400a9

JakenVeina reviewed Apr 1, 2024

View reviewed changes

RolandPheasant added 2 commits June 3, 2024 08:35

Merge branch 'main' into feature/virtualise-sort-bind

3a3dfe1

Respond to PR feedback for sort and virtualize.

28fb2dc

RolandPheasant merged commit 275cc2c into main Jun 3, 2024
1 check passed

RolandPheasant deleted the feature/virtualise-sort-bind branch June 3, 2024 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Virtualization operator #888

New Virtualization operator #888

RolandPheasant commented Mar 28, 2024

RolandPheasant Mar 28, 2024

RolandPheasant Mar 28, 2024

dwcullop Mar 28, 2024

JakenVeina Apr 1, 2024

dwcullop Mar 28, 2024

dwcullop Mar 28, 2024

RolandPheasant Mar 28, 2024

JakenVeina Apr 1, 2024

dwcullop Mar 28, 2024

RolandPheasant Mar 28, 2024

dwcullop Mar 28, 2024

RolandPheasant Mar 28, 2024

RolandPheasant Mar 28, 2024

RolandPheasant Jun 3, 2024

dwcullop Mar 28, 2024

RolandPheasant Mar 28, 2024

JakenVeina Apr 1, 2024

JakenVeina Apr 1, 2024

RolandPheasant Jun 3, 2024

JakenVeina Apr 1, 2024

JakenVeina Apr 1, 2024

JakenVeina Apr 1, 2024

JakenVeina Apr 1, 2024

New Virtualization operator #888

New Virtualization operator #888

Conversation

RolandPheasant commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment