Cache PropertyChangedEventArgs

[JohanLarsson]

From @JohanLarsson on January 3, 2017 13:47

internal static class PropertyChangedArgsFor
{
    internal static readonly PropertyChangedEventArgs Value = Create();

    private static PropertyChangedEventArgs Create([CallerMemberName] string propertyName = null)
    {
        return new PropertyChangedEventArgs(propertyName);
    }
}

usage:

public int Value
{
    get
    {
        return this.value;
    }

    protected set
    {
        if (value == this.value)
        {
            return;
        }

        this.value= value;
        this.PropertyChanged?.Invoke(this, PropertyChangedArgsFor.Value);
    }
}

Copied from original issue: DotNetAnalyzers/WpfAnalyzers#116

[JohanLarsson]

Maybe internal static class PropertyNamed

[JohanLarsson]

From @Venomed on January 3, 2017 14:3

No i like the PropertyChangedArgsFor better though as I said in chat, I don't like leaking implementation detail out of class like that. Caching is good but it should be within the class it's meant for. If you had a shared static class like that and you did a refactor, there's a chance that a property rename could affect more classes than you intended because of code reuse

[JohanLarsson]

A rename should not be an issue as we have analyzers checking that we notify for the correct name.

[JohanLarsson]

From @sharwell on January 3, 2017 14:25

Another way to implement this is using a ConcurrentDictionary<string, PropertyChangedEventArgs>.

internal static class PropertyChangedArgsCache
{
  private static readonly ConcurrentDictionary<string, PropertyChangedEventArgs> cache =
    new ConcurrentDictionary<string, PropertyChangedEventArgs>();

  public static PropertyChangedEventArgs Get(string propertyName)
  {
    return cache.GetOrCreate(propertyName, property => new PropertyChangedEventArgs(property));
  }
}

Usage

public int Value
{
    get
    {
        return this.value;
    }

    protected set
    {
        if (value == this.value)
        {
            return;
        }

        this.value= value;
        this.PropertyChanged?.Invoke(this, PropertyChangedArgsCache.Get(nameof(Value)));
    }
}

You could also combine the approaches - use the PropertyChangedArgsCache to initialize private static fields of the current type. This would minimize the number of PropertyChangedEventArgs instances created, avoid the potential refactoring problems, and maximize runtime performance, but would have the downside of creating many new fields (ugly).

[JohanLarsson]

Ran a quick benchmark:

public class PropertyChangedEventArgsBenchmarks
{
    private static readonly PropertyChangedEventArgs Cached = new PropertyChangedEventArgs("Foo");
    private static readonly ConcurrentDictionary<string, PropertyChangedEventArgs> Cache = new ConcurrentDictionary<string, PropertyChangedEventArgs>();

    [Benchmark(Baseline = true)]
    public PropertyChangedEventArgs NewPropertyChangedEventArgs()
    {
        return new PropertyChangedEventArgs("Foo");
    }

    [Benchmark]
    public PropertyChangedEventArgs ReturnCached()
    {
        return Cached;
    }

    [Benchmark]
    public PropertyChangedEventArgs ReturnCachedInDictionary()
    {
        return Cache.GetOrAdd("Foo", x => new PropertyChangedEventArgs(x));
    }
}

                      Method |       Mean |    StdErr |    StdDev |     Median | Scaled | Scaled-StdDev |  Gen 0 | Allocated |
---------------------------- |----------- |---------- |---------- |----------- |------- |-------------- |------- |---------- |
 NewPropertyChangedEventArgs |  4.3362 ns | 0.0500 ns | 0.1937 ns |  4.4381 ns |   1.00 |          0.00 | 0.0019 |      12 B |
                ReturnCached |  0.1027 ns | 0.0251 ns | 0.0973 ns |  0.0877 ns |   0.02 |          0.02 |      - |       0 B |
    ReturnCachedInDictionary | 40.6604 ns | 0.4320 ns | 2.9931 ns | 39.2088 ns |   9.40 |          0.81 |      - |       0 B |

It probably underestimates the cost of new as i the cost of GC comes later. It also gives a low number for dictionary as the dictionary has only one item. A dictionary lookup is still fast compared to the updates in the view that the event may cause.

Regarding memory: The ConcurrentDictionary<string, PropertyChangedEventArgs> is an allocation of 868 B that we pay for from the start. That is 72 PropertyChangedEventArgs with the string "Foo"

[Charles113]

We would also need to be able to change the PropertyName, PropertyName is ReadOnly. And would have to use [ThreadStatic]

But yeah i also don't understand why we have to allocate so much stuff for a common case like property changed. Imagine 100k objects with 20 props, allocating 2 million new PropertyChangedEventArgs() objects ? Seems like a weird design choice by 2024.

[Maverik]

We would also need to be able to change the PropertyName, PropertyName is ReadOnly. And would have to use [ThreadStatic]

But yeah i also don't understand why we have to allocate so much stuff for a common case like property changed. Imagine 100k objects with 20 props, allocating 2 million new PropertyChangedEventArgs() objects ? Seems like a weird design choice by 2024.

Might be good to start a new issue perhaps, I'd also be curious to hear why would you want to have 100K objects all allocation PropertyChangedEventArgs? Practically speaking I'd have engaged data virtualisation probably even at a 100 item mark as INPC is typically a UI concern only and you wouldn't have more than a few 10s of items on screen at any given time. Are you using it differently?

[Charles113]

Might be good to start a new issue perhaps, I'd also be curious to hear why would you want to have 100K objects all allocation PropertyChangedEventArgs? Practically speaking I'd have engaged data virtualisation probably even at a 100 item mark as INPC is typically a UI concern only and you wouldn't have more than a few 10s of items on screen at any given time. Are you using it differently?

I guess it depends on the way your framework implements INotifyPropertyChanged, so maybe a non issue. I was thinking of loading 100k objects that all might trigger the property changed event. For example if you use Attributes where you don't have much control over the generated source code.
I already tested [ThreadStatic] for caching, but it's not an option, as its slower than just allocating new PropertyChangedEventArgs.

I think it would be nice to cache it, but i can't think of any way that's faster than creating new PropertyChangedEventArgs(). It just seems like a waste, as by default only the prop name changes.