Keypath filtering (#3179)

Co-authored-by: Jacob Gunnarsson <jacob.gunnarsson@mongodb.com>

CHANGELOG.md

@@ -12,15 +12,17 @@
 * Upgrade OpenSSL from 1.1.1n to 3.0.7. (Core 13.2.0)
 * Converting flexible sync realms to bundled and local realms is now supported (Core 13.2.0)
 * Add support for nested classes for source generated classes. (Issue [#3031](https://github.com/realm/realm-dotnet/issues/3031))
+* Improved performance of `PropertyChanged` and `CollectionChanged` notifications. (Issue [#3112](https://github.com/realm/realm-dotnet/issues/3112))
 
 ### Fixed
-* Set<Mixed> consider string and binary data equivalent. This could cause the client to be inconsistent with the server if a string and some binary data with equivalent content was inserted from Atlas. (Core 13.0.0)
+* `ISet<RealmValue>` consider string and binary data equivalent. This could cause the client to be inconsistent with the server if a string and some binary data with equivalent content was inserted from Atlas. (Core 13.0.0)
 * Fixed wrong assertion on query error that could result in a crash. (Core 13.1.0)
 * Fixed an issue preventing opening an encrypted file on a device with a page size bigger than the one on which the file was produced. (Core 13.1.1)
 * Fixed possible segfault in sync client where async callback was using object after being deallocated (Core 13.2.0)
 * Fixed crash when using client reset with recovery and flexible sync with a single subscription (Core 13.2.0)
 * Added a more descriptive error message when a model's property is unsupported. It'll now suggest that the target type may need to inherit from `RealmObject`. (Issue [#3162](https://github.com/realm/realm-dotnet/issues/3162))
 * Disposing a Realm instance while an active transaction is running will now correctly roll back the transaction. (Issue [#2924](https://github.com/realm/realm-dotnet/issues/2924))
+* Fixed an issue that would cause `PropertyChanged` notifications to be delivered for collection properties when the content of the collection was modified even if the collection itself was not replaced. (Issue [#3112](https://github.com/realm/realm-dotnet/issues/3112))
 
 ### Compatibility
 * Realm Studio: 13.0.0 or later.

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs

@@ -199,7 +199,7 @@ public void UnsubscribeFromNotifications()
         }
 
         /// <inheritdoc/>
-        void INotifiable<NotifiableObjectHandleBase.CollectionChangeSet>.NotifyCallbacks(NotifiableObjectHandleBase.CollectionChangeSet? changes)
+        void INotifiable<NotifiableObjectHandleBase.CollectionChangeSet>.NotifyCallbacks(NotifiableObjectHandleBase.CollectionChangeSet? changes, bool shallow)
         {
             if (changes.HasValue)
             {

Realm/Realm/DatabaseTypes/INotifiable.cs

@@ -33,7 +33,8 @@ internal interface INotifiable<TChangeset>
         /// Method called when there are changes to report for that object.
         /// </summary>
         /// <param name="changes">The changes that occurred.</param>
-        void NotifyCallbacks(TChangeset? changes);
+        /// <param name="shallow">Whether the changes are coming from a shallow notifier or not.</param>
+        void NotifyCallbacks(TChangeset? changes, bool shallow);
     }
 
     internal class NotificationToken<TCallback> : IDisposable

Realm/Realm/DatabaseTypes/Metadata.cs

@@ -42,5 +42,15 @@ public Metadata(TableKey tableKey, IRealmObjectHelper helper, IDictionary<string
             PropertyIndices = new ReadOnlyDictionary<string, IntPtr>(propertyIndices);
             Schema = schema;
         }
+
+        public IntPtr GetPropertyIndex(string propertyName)
+        {
+            if (PropertyIndices.TryGetValue(propertyName, out var result))
+            {
+                return result;
+            }
+
+            throw new MissingMemberException(Schema.Name, propertyName);
+        }
     }
 }

Realm/Realm/DatabaseTypes/RealmCollectionBase.cs

@@ -43,11 +43,7 @@ public abstract class RealmCollectionBase<T>
           IThreadConfined,
           IMetadataObject
     {
-        private readonly List<NotificationCallbackDelegate<T>> _callbacks = new();
-
-        private NotificationTokenHandle _notificationToken;
-
-        private bool _deliveredInitialNotification;
+        private readonly Lazy<NotificationCallbacks<T>> _notificationCallbacks;
 
         internal readonly bool _isEmbedded;
 
@@ -149,11 +145,15 @@ internal RealmCollectionBase(Realm realm, Metadata metadata)
             Handle = new Lazy<CollectionHandleBase>(GetOrCreateHandle);
             Metadata = metadata;
             _isEmbedded = metadata?.Schema.BaseType == ObjectSchema.ObjectType.EmbeddedObject;
+            _notificationCallbacks = new(() => new(this));
         }
 
         ~RealmCollectionBase()
         {
-            UnsubscribeFromNotifications();
+            if (_notificationCallbacks.IsValueCreated)
+            {
+                _notificationCallbacks.Value.RemoveAll();
+            }
         }
 
         internal abstract CollectionHandleBase GetOrCreateHandle();
@@ -180,21 +180,14 @@ public IRealmCollection<T> Freeze()
         }
 
         public IDisposable SubscribeForNotifications(NotificationCallbackDelegate<T> callback)
+            => SubscribeForNotificationsImpl(callback, shallow: false);
+
+        internal IDisposable SubscribeForNotificationsImpl(NotificationCallbackDelegate<T> callback, bool shallow)
         {
             Argument.NotNull(callback, nameof(callback));
+            _notificationCallbacks.Value.Add(callback, shallow);
 
-            if (_callbacks.Count == 0)
-            {
-                SubscribeForNotifications();
-            }
-            else if (_deliveredInitialNotification)
-            {
-                callback(this, null, null);
-            }
-
-            _callbacks.Add(callback);
-
-            return NotificationToken.Create(callback, UnsubscribeFromNotifications);
+            return NotificationToken.Create(callback, c => UnsubscribeFromNotifications(c, shallow));
         }
 
         protected abstract T GetValueAtIndex(int index);
@@ -254,31 +247,9 @@ protected static IEmbeddedObject EnsureUnmanagedEmbedded(in RealmValue value)
             return result;
         }
 
-        private void UnsubscribeFromNotifications(NotificationCallbackDelegate<T> callback)
-        {
-            if (_callbacks.Remove(callback) &&
-                _callbacks.Count == 0)
-            {
-                UnsubscribeFromNotifications();
-            }
-        }
-
-        private void SubscribeForNotifications()
-        {
-            Debug.Assert(_notificationToken == null, "_notificationToken must be null before subscribing.");
-
-            Realm.ExecuteOutsideTransaction(() =>
-            {
-                var managedResultsHandle = GCHandle.Alloc(this, GCHandleType.Weak);
-                _notificationToken = Handle.Value.AddNotificationCallback(GCHandle.ToIntPtr(managedResultsHandle));
-            });
-        }
-
-        private void UnsubscribeFromNotifications()
+        private void UnsubscribeFromNotifications(NotificationCallbackDelegate<T> callback, bool shallow)
         {
-            _notificationToken?.Dispose();
-            _notificationToken = null;
-            _deliveredInitialNotification = false;
+            _notificationCallbacks.Value.Remove(callback, shallow);
         }
 
         #region INotifyCollectionChanged
@@ -410,18 +381,18 @@ private void UpdateCollectionChangedSubscriptionIfNecessary(bool isSubscribed)
             {
                 if (isSubscribed)
                 {
-                    SubscribeForNotifications(OnChange);
+                    SubscribeForNotificationsImpl(OnChange, shallow: true);
                 }
                 else
                 {
-                    UnsubscribeFromNotifications(OnChange);
+                    UnsubscribeFromNotifications(OnChange, shallow: true);
                 }
             }
         }
 
         #endregion INotifyCollectionChanged
 
-        void INotifiable<NotifiableObjectHandleBase.CollectionChangeSet>.NotifyCallbacks(NotifiableObjectHandleBase.CollectionChangeSet? changes)
+        void INotifiable<NotifiableObjectHandleBase.CollectionChangeSet>.NotifyCallbacks(NotifiableObjectHandleBase.CollectionChangeSet? changes, bool shallow)
         {
             ChangeSet changeset = null;
             if (changes != null)
@@ -435,15 +406,8 @@ private void UpdateCollectionChangedSubscriptionIfNecessary(bool isSubscribed)
                     moves: actualChanges.Moves.AsEnumerable().Select(m => new ChangeSet.Move((int)m.From, (int)m.To)).ToArray(),
                     cleared: actualChanges.Cleared);
             }
-            else
-            {
-                _deliveredInitialNotification = true;
-            }
 
-            foreach (var callback in _callbacks.ToArray())
-            {
-                callback(this, changeset, null);
-            }
+            _notificationCallbacks.Value.Notify(changeset, shallow);
         }
 
         public IEnumerator<T> GetEnumerator() => new Enumerator(this);
@@ -624,6 +588,102 @@ internal interface IRealmCollectionBase<THandle> : IMetadataObject
         THandle NativeHandle { get; }
     }
 
+    [SuppressMessage("StyleCop.CSharp.MaintainabilityRules", "SA1402:File may only contain a single type", Justification = "NotificationCallbacks are tighly coupled with the collection and it's easier to reason about when they're in the same file.")]
+    internal class NotificationCallbacks<T>
+    {
+        private readonly RealmCollectionBase<T> _parent;
+
+        private readonly Dictionary<KeyPath, (NotificationTokenHandle Token, bool DeliveredInitialNotification, List<NotificationCallbackDelegate<T>> Callbacks)> _subscriptions = new();
+
+        public NotificationCallbacks(RealmCollectionBase<T> parent)
+        {
+            _parent = parent;
+        }
+
+        // Shallow here and everywhere else is a bit of a shortcut we're taking until we have proper keypath filtering.
+        // Once we do, we probably want this to be some keypath identifier that we can pass between managed and native.
+        public void Add(NotificationCallbackDelegate<T> callback, bool shallow)
+        {
+            var keyPath = shallow ? KeyPath.Empty : KeyPath.Full;
+            if (_subscriptions.TryGetValue(keyPath, out var subscription))
+            {
+                if (subscription.DeliveredInitialNotification)
+                {
+                    // If Core already delivered the initial notification, we need to manually invoke the callback as it won't be invoked by Core.
+                    // It's part of the SubscribeForNotifications API contract that an initial callback with `null` changes is always delivered.
+                    callback(_parent, null, null);
+                }
+
+                // If we have a subscription already, we just add the callback to the list we're managing
+                subscription.Callbacks.Add(callback);
+            }
+            else
+            {
+                // If this is a new subscription, we store it in the backing dictionary, then we subscribe outside of transaction
+                _subscriptions[keyPath] = (null, false, new() { callback });
+                _parent.Realm.ExecuteOutsideTransaction(() =>
+                {
+                    // It's possible that we unsubscribed in the meantime, so only add a notification callback if we still have callbacks
+                    if (_subscriptions.TryGetValue(keyPath, out var subscription) && subscription.Callbacks.Count > 0)
+                    {
+                        var managedResultsHandle = GCHandle.Alloc(_parent, GCHandleType.Weak);
+                        _subscriptions[keyPath] = (_parent.Handle.Value.AddNotificationCallback(GCHandle.ToIntPtr(managedResultsHandle), shallow), subscription.DeliveredInitialNotification, subscription.Callbacks);
+                    }
+                });
+            }
+        }
+
+        public bool Remove(NotificationCallbackDelegate<T> callback, bool shallow)
+        {
+            var keyPath = shallow ? KeyPath.Empty : KeyPath.Full;
+            if (_subscriptions.TryGetValue(keyPath, out var subscription))
+            {
+                subscription.Callbacks.Remove(callback);
+                if (subscription.Callbacks.Count == 0)
+                {
+                    subscription.Token?.Dispose();
+                    _subscriptions.Remove(keyPath);
+                }
+
+                return true;
+            }
+
+            return false;
+        }
+
+        public void Notify(ChangeSet changes, bool shallow)
+        {
+            var keyPath = shallow ? KeyPath.Empty : KeyPath.Full;
+            if (_subscriptions.TryGetValue(keyPath, out var subscription))
+            {
+                if (changes == null)
+                {
+                    _subscriptions[keyPath] = (subscription.Token, true, subscription.Callbacks);
+                }
+
+                foreach (var callback in subscription.Callbacks.ToArray())
+                {
+                    callback(_parent, changes, null);
+                }
+            }
+        }
+
+        public void RemoveAll()
+        {
+            foreach (var token in _subscriptions.Values.Select(c => c.Token))
+            {
+                token.Dispose();
+            }
+        }
+    }
+
+    // Eventually this should be a proper class holding the keypaths
+    internal enum KeyPath
+    {
+        Full,
+        Empty
+    }
+
     /// <summary>
     /// Special invalid object that is used to avoid an exception in WPF
     /// when deleting an element from a collection bound to UI (<see href="https://github.com/realm/realm-dotnet/issues/1903">#1903</see>).

Realm/Realm/DatabaseTypes/RealmDictionary.cs

@@ -42,7 +42,7 @@ public class RealmDictionary<TValue>
           IRealmCollectionBase<DictionaryHandle>,
           INotifiable<DictionaryHandle.DictionaryChangeSet>
     {
-        private readonly List<DictionaryNotificationCallbackDelegate<TValue>> _keyCallbacks = new List<DictionaryNotificationCallbackDelegate<TValue>>();
+        private readonly List<DictionaryNotificationCallbackDelegate<TValue>> _keyCallbacks = new();
         private readonly DictionaryHandle _dictionaryHandle;
 
         private bool _deliveredInitialKeyNotification;
@@ -249,8 +249,10 @@ private static string ValidateKey(string key)
 
         protected override KeyValuePair<string, TValue> GetValueAtIndex(int index) => _dictionaryHandle.GetValueAtIndex<TValue>(index, Realm);
 
-        void INotifiable<DictionaryHandle.DictionaryChangeSet>.NotifyCallbacks(DictionaryHandle.DictionaryChangeSet? changes)
+        void INotifiable<DictionaryHandle.DictionaryChangeSet>.NotifyCallbacks(DictionaryHandle.DictionaryChangeSet? changes, bool shallow)
         {
+            Debug.Assert(!shallow, "Shallow should always be false here as we don't expose a way to configure it.");
+
             DictionaryChangeSet changeset = null;
             if (changes != null)
             {

Realm/Realm/Handles/CollectionHandleBase.cs

@@ -61,5 +61,7 @@ public ResultsHandle GetFilteredResults(string query, RealmValue[] arguments)
         protected abstract IntPtr GetFilteredResultsCore(string query, PrimitiveValue[] arguments, out NativeException ex);
 
         protected virtual IntPtr SnapshotCore(out NativeException ex) => throw new NotSupportedException("Snapshotting this collection is not supported.");
+
+        public abstract NotificationTokenHandle AddNotificationCallback(IntPtr managedObjectHandle, bool shallow);
     }
 }

Realm/Realm/Handles/DictionaryHandle.cs

@@ -34,7 +34,7 @@ internal struct DictionaryChangeSet
         }
 
         [UnmanagedFunctionPointer(CallingConvention.Cdecl)]
-        public delegate void KeyNotificationCallback(IntPtr managedHandle, IntPtr changes);
+        public delegate void KeyNotificationCallback(IntPtr managedHandle, IntPtr changes, [MarshalAs(UnmanagedType.U1)] bool shallow);
 
         private static class NativeMethods
         {
@@ -48,7 +48,7 @@ private static class NativeMethods
             public static extern void destroy(IntPtr listInternalHandle);
 
             [DllImport(InteropConfig.DLL_NAME, EntryPoint = "realm_dictionary_add_notification_callback", CallingConvention = CallingConvention.Cdecl)]
-            public static extern IntPtr add_notification_callback(DictionaryHandle handle, IntPtr managedDictionaryHandle, out NativeException ex);
+            public static extern IntPtr add_notification_callback(DictionaryHandle handle, IntPtr managedDictionaryHandle, [MarshalAs(UnmanagedType.U1)] bool shallow, out NativeException ex);
 
             [DllImport(InteropConfig.DLL_NAME, EntryPoint = "realm_dictionary_add_key_notification_callback", CallingConvention = CallingConvention.Cdecl)]
             public static extern IntPtr add_key_notification_callback(DictionaryHandle handle, IntPtr managedDictionaryHandle, out NativeException ex);
@@ -136,11 +136,11 @@ public override void Clear()
             nativeException.ThrowIfNecessary();
         }
 
-        public override NotificationTokenHandle AddNotificationCallback(IntPtr managedObjectHandle)
+        public override NotificationTokenHandle AddNotificationCallback(IntPtr managedObjectHandle, bool shallow)
         {
             EnsureIsOpen();
 
-            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, out var nativeException);
+            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, shallow, out var nativeException);
             nativeException.ThrowIfNecessary();
             return new NotificationTokenHandle(Root, result);
         }
@@ -338,11 +338,11 @@ public ResultsHandle GetKeys()
         }
 
         [MonoPInvokeCallback(typeof(KeyNotificationCallback))]
-        public static void NotifyDictionaryChanged(IntPtr managedHandle, IntPtr changes)
+        public static void NotifyDictionaryChanged(IntPtr managedHandle, IntPtr changes, bool shallow)
         {
             if (GCHandle.FromIntPtr(managedHandle).Target is INotifiable<DictionaryChangeSet> notifiable)
             {
-                notifiable.NotifyCallbacks(new PtrTo<DictionaryChangeSet>(changes).Value);
+                notifiable.NotifyCallbacks(new PtrTo<DictionaryChangeSet>(changes).Value, shallow);
             }
         }
     }

Realm/Realm/Handles/ListHandle.cs

@@ -17,6 +17,7 @@
 ////////////////////////////////////////////////////////////////////////////
 
 using System;
+using System.Collections.Generic;
 using System.Runtime.InteropServices;
 using Realms.Native;
 
@@ -63,7 +64,7 @@ private static class NativeMethods
             public static extern void destroy(IntPtr listInternalHandle);
 
             [DllImport(InteropConfig.DLL_NAME, EntryPoint = "list_add_notification_callback", CallingConvention = CallingConvention.Cdecl)]
-            public static extern IntPtr add_notification_callback(ListHandle listHandle, IntPtr managedListHandle, out NativeException ex);
+            public static extern IntPtr add_notification_callback(ListHandle listHandle, IntPtr managedListHandle, [MarshalAs(UnmanagedType.U1)] bool shallow, out NativeException ex);
 
             [DllImport(InteropConfig.DLL_NAME, EntryPoint = "list_move", CallingConvention = CallingConvention.Cdecl)]
             public static extern IntPtr move(ListHandle listHandle, IntPtr sourceIndex, IntPtr targetIndex, out NativeException ex);
@@ -213,11 +214,11 @@ public void Move(IntPtr sourceIndex, IntPtr targetIndex)
             nativeException.ThrowIfNecessary();
         }
 
-        public override NotificationTokenHandle AddNotificationCallback(IntPtr managedObjectHandle)
+        public override NotificationTokenHandle AddNotificationCallback(IntPtr managedObjectHandle, bool shallow)
         {
             EnsureIsOpen();
 
-            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, out var nativeException);
+            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, shallow, out var nativeException);
             nativeException.ThrowIfNecessary();
             return new NotificationTokenHandle(Root, result);
         }