Saturday, March 26, 2011

Raising Property Changed fast and safe - yet another solution

This Thursday we discussed with my colleagues how to raise property changed event. We learned that it could be done
a) via string literal,
b) via lambda expression tree,
c) via StackTrace or MethodBase.GetCurrentMethod().
Nice summary is here. or here

We did some naive performance test and learned that for 500k iterations on simple property, there is big difference in speed.
a) is really fast, 13ms
b) is slow, 1417ms, I guess because security checks apply for every call and every stack frame, and can't be fired from other properties.
c) is slow, 2518ms, very slow, because expression tree is constructed for every call

So we played with it for a while and I invented solution with anonymous type (at botom).

Update 27.3.2011: I was so focused on speed that I didn't realized that the solution yesterday didn't work nicely for rename. What a shame. I hope people give me another chance today. Here we have postponed creation of the expression tree, it takes about 75ms. And this time my ReSharper renames it correctly :-)
Update 5.12.2011: As Alex noted in comments, there was thread safety issue with Dictionary. The code is now fixed with Hashtable instead.


public abstract class ModelBase : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;
    private static readonly Hashtable Names = new Hashtable();

    protected void RaisePropertyChange(Func<string> key)
    {
        if (PropertyChanged != null)
        {
            var propertyName = (PropertyChangedEventArgs)Names[key];
            if (propertyName==null)
            {
                propertyName = new PropertyChangedEventArgs(key());
                lock (Names)
                {
                    Names[key] = propertyName;
                }
            }
            PropertyChanged(this, propertyName);
        }
    }

    protected static string Reg<T>(Expression<Func<T>> property)
    {
        return (property.Body as MemberExpression).Member.Name;
    }
}

public class Model : ModelBase
{
    private string firstName2;
    public string FirstName2
    {
        get { return firstName2; }
        set
        {
            firstName2 = value;
            RaisePropertyChange(() => Reg(() => FirstName2));
        }
    }
}

Doesn't work: The lambda just helps to define the type without creating the instance and calling the getter. It takes about 77ms for half million calls.

public abstract class ModelBase : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;
        private static readonly Dictionary<Type, PropertyChangedEventArgs> Names = 
          new Dictionary<Type, PropertyChangedEventArgs>();

        protected void RaisePropertyChange<T>(Func<T> f)
        {
            if (PropertyChanged!=null)
            {
                var key = typeof(T);
                PropertyChangedEventArgs evntArgs;
                if (!Names.TryGetValue(key, out evntArgs))
                {
                    var propertyName = key.GetProperties()[0].Name;
                    evntArgs = new PropertyChangedEventArgs(propertyName);
                    lock(Names)
                    {
                        Names[key] = evntArgs;
                    }
                }
                PropertyChanged(this, evntArgs);
            }
        }
    }

    public class Model : ModelBase
    {
        private string firstName;
        public string FirstName
        {
            get { return firstName; }
            set {
                if (firstName != value)
                {
                    firstName = value;
                    RaisePropertyChange(() => new {FirstName});
                }
            }
        }
    }

13 comments:

John Rusk said...

Clever!

Phil Parker said...

If you're doing what I think you're doing, I had the same idea, and patted myself on the back at the time for I thought a very clever idea.

Unfortunately it does not work.

It's not re-factoring safe - when you rename the property 'FirstName' the anonymous member does not get renamed, so if for example, you rename 'FirstName' to 'Name', after refactoring you'll get "new { FirstName = Name }"

Thus defeating the whole purpose - to avoid strings and be refactoring safe. Since the name is derived from the anonymous type's member not the original property.

Was very disappointed myself, at the time, because I thought it was a really cool idea.

Phil

John Rusk said...

@Phil Good point. I didn't realise that when I first read this post and left my previous comment.

@Pavel Thanks for posting the interesting timings. I've done some timings of my own and got similar results. All in ms per 500,000 calls:

SimpleString : 11ms
StringWithStaticArgs : 4ms
StackCrawl : 15407ms
ActiveSharpDirect : 77ms

The last one, in case you're interested, is from my project at ActiveSharp.Codeplex.com. It's not a perfect solution (has some very complex one-off initialization code) but is faster and safer than stack crawls.

(By the way, I'm not quite sure why my stack crawl one came out 10 times slower than yours...)

Simon Cropp said...

Try ILWeaving instead
http://visualstudiogallery.msdn.microsoft.com/bd351303-db8c-4771-9b22-5e51524fccd3

David Keaveny said...

This is also a good use of AOP (either through Postsharp or Unity); then your properties can be standard auto-properties, and you let the AOP framework take care of wiring up your IPNC listeners.

David Keaveny said...

http://khason.net/dev/inotifypropertychanged-auto-wiring-or-how-to-get-rid-of-redundant-code/ gives an example of using Postsharp to achieve this; while http://shecht.wordpress.com/2009/12/12/inotifypropertychanged-with-unity-interception-aop/ does the same for Unity.

Pavel Šavara said...

@Phil, my bad :( I updated the article with yet another solution we drafted during initial brainstorming.

@Simon, @David, I know about AOP/weaving, that's obviously best solution, if you could use it.

Phil Parker said...

Pavel, have you benchmarked your latest solution?

I tried some techniques myself to avoid the performance hit from expression trees, and I think I tried something like what (I think!) you seem to be doing here (attempting to only evaluate the expression tree once?) but I always ended up taking the performance hit.

The solution I eventually came up with was to use a combination of expression trees, but cache them in static PropertyChangedEventArgs variables - thus refactoring safe, but without any performance hit.

I eventually ditched this since the resultant code was too painful. Right now I'm using straight expression trees since performance isn't too critical, but having tried out postsharp, that's eventually going to be my go forward solution. Way nicer!

Phil

Pavel Šavara said...

@Phil, yes I measured it, it's around 75ms. The slow part is to create the expression tree, not to parse it I believe. I updated the code to cache PropertyChangedEventArgs.

Regarding the impact on overall speed of the application, you are right, in most cases it just doesn't matter. I can imagine that we have list of viewModel with 10k rows and changing them all. In such case user may notice it. We should be pragmatic and balance all concerns, not only speed.

Phil Parker said...

That's very nice in that case Pavel - I was never able to avoid the performance hit. I may use your technique when PostSharp isn't an option.

Cheers :)

Phil

Alex Davies said...

Pavel, your implementation is not thread-safe - yet it looks like it's trying to be (the worst kind of thread non-safety!).

If you replace your generic Dictionary with a Hashtable and a few casts (which are plenty quick enough) this would be fine. You don't really need the generics in this case anyway.

Reason? Hashtable supports concurrent readers with a single writing thread, so your lock around the write makes sense.

Dictionary however offers *no* thread safety guarantees, and so must not be read whilst it's being written to - the lock would have to be around the entire method to be effective.

Pavel Šavara said...

@Alex thanks for that. Fixed.

Bruce said...

This is clever, deferring the evaluation and caching the PropertyChangedEventArgs, but... your hashtable is gonna get HUGE, because you are using the Func as the key for the hash.

The appropriate key is Names[key.Method.ToString()]. This will be unique per property, even if the property is the same name on different classes. And even if it ends up not being completely unique, it will still return the correct string.

By the way: your captcha is completely ridiculous, almost impossible to read.