CODEWITHSUNDEEP
c#.netmvvminotifypropertychanged
Stepagrus
Stepagrus

Reputation: 1559

PropertyChangedEventArgs pooling

I want to decide if there is a benefit to doing a pool of PropertyChangedEventArgs objects.

(For those who are not in the subject, I will explain - the PropertyChangedEventArgs object is part of the INotifyPropertyChanged interface of the MVVM pattern)

For simple example:

public class ObservableObject : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler? PropertyChanged;
    protected void OnPropertyChanged([CallerMemberName] string? propertyName = null)
    {
        var arg = _pool
            .GetOrAdd(propertyName, name => new PropertyChangedEventArgs(name));
        PropertyChanged?.Invoke(this, arg);
    }

    private readonly static ConcurrentDictionary<string, PropertyChangedEventArgs> _pool 
        = new();
}

I would like to reduce the load on the GC, but at the same time, the String.GetHashCode() method is not cached and is calculated every time, which increases the load on the CPU.

What do you think about this question?

Upvotes: 1

Views: 209

Answers (2)

Emperor Eto
Emperor Eto

Reputation: 3520

I can't say I've done research here but I have lots of experience with INPC including in contexts outside of XAML. Specifically for my web application I use a homegrown MVVM framework that combines React with C#/WASM via my own binding framework. Performance, especially with INPCs, is thus crucial for me.

You're talking here about the pros and cons of creating a new PropertyChangedEventArgs instance for every property change notification, versus caching the args in a ConcurrentDictionary.

Creating new args requires, for each property change -

  • Allocating a small amount of memory on the global heap and later GC'ing it. Remember that .NET caches all hard-coded strings anyway (which your property names would be) so you're not actually allocating extra heap space for the string itself each time, just for the args instance and the pointer to the string. These are all very cheap operations.

Your approach requires, for each property change -

  • Computing the hash for the property name string
  • Locking the thread (as you're using a ConcurrentDictionary) - cheap but not free
  • An O(logN) dictionary operation to either find the existing args or add a new instance to the dict. Obviously as you add more and more properties that have change notifications this is going to not be negligible.

Again without running performance benchmarks, my experience tells me that approach #2 is going to be quite significantly more of a CPU burden than approach #1.

But if you do the performance benchmarks I hope you post the results, would be very curious.

Also though it's not a direct answer, if you really wanted to squeeze every possible last bit of performance out of this, you could create a static PropertyChangedEventArgs field for each property in each viewmodel class and simply raise PropertyChanged in the setter using that static instance. More lines of code, but it would accomplish your goal. I'd be surprised if it resulted in a noticeable performance improvement though.

Finally here's a "cute" option - keep a cache of args but as a simple growable list, keyed to the line number of the code. (Note this won't work with partial classes!!)

    private static GrowableList<PropertyChangedEventArgs> _argCache;
    private void OnPropertyChanged(
        [CallerMemberName] 
        string property = null,
        [CallerLineNumber] 
        int key = 0)
    {
        var args = _argCache[key] ?? (_argCache[key] = new PropertyChangedEventArgs(property));
        this.PropertyChanged?.Invoke(this, args);
    }

GrowableList would just be a list-type object that automatically grew to the given index and inserted null values as needed.

Of course it's wasteful of RAM, but only once per class, but as it would just use array indexing it would be nearly as fast as individual static fields.

You could also combine the approaches and key a dictionary with the code line number, which would also be a little faster than using the property name.

Upvotes: 1

Marc Gravell
Marc Gravell

Reputation: 1062494

IMO the more useful thing to do here is not do anything if no-one is listening:

var handler = PropertyChanged;
if (handler is not null)
{
    var arg = // TODO: your choice of implementation here
    handler.Invoke(this, arg);
}

This can also be shortened:

PropertyChanged?.Invoke(this, /* TODO, initialize inline */);

Upvotes: 1

Related Questions