Alternative to ObservableObject

Jul 20, 2009 at 4:21 PM

Josh, have you considered as an alternative to your ObservableObject, using lambda's and expressions, similar to how you've implemented your PropertyObserver class?

I've seen a couple of postings which use this technique, and the advantages would appear to be: removal of string literals (same as for your PropertyObserver), and secondly removal of the need to inherit your view model classes from a particular base class.  (For me any time you can avoid implementation inheritance is good).

Here are the links:

http://www.ingebrigtsen.info/post/2008/12/11/INotifyPropertyChanged-revisited.aspx

http://www.ingebrigtsen.info/post/2009/07/11/Extensions-and-Helpers-for-Silverlight-and-WPF.aspx

http://blogs.microsoft.co.il/blogs/dorony/archive/2007/08/31/WPF-Binding_2C00_-INotifyPropertyChanged-and-Linq.aspx

I'd be interested to know what you think - I can see some possible performance issues, but all things being equal avoiding string literals and base class requirements seems very attractive.

Regards,

Phil

Coordinator
Jul 20, 2009 at 4:38 PM

Phil,

I considered using lambdas to provide property names to ObservableObject's RaisePropertyChanged method, but decided not to implement it due to the performance implications.  Since you only register handlers with PropertyObserver once per instance, the performance consideration is not very relevant. Property change notifications can happen very frequently and consistently throughout the life of an object.

If someone provides me with performance test results showing that using the lambda approach is not all that bad, I might reconsider. 

Thanks,

Josh

Jul 20, 2009 at 7:03 PM
Edited Jul 20, 2009 at 7:04 PM

Josh, I'm taking a look at it r.e. performace; there certainly is a hit - extremely rough tests so far indicate approx a 30x overhead.

However, what's interesting is that the vast majority of that appears to be in the reference to the lambda expression itself - 

If instead of directly including the lambda, you cache a single instance of it as member data you can bring it down to a 2x overhead.  For example:

Instead of:

        public string TextProperty
        {
            get { return _textProperty; }
            set
            {
                _textProperty = value;
                PropertyChanged.Notify(m_textPropExpression);
                //PropertyChanged.Notify(() => TextProperty);
            }
        }

 

 

        public string TextProperty

        {

            get { return _textProperty; }

            set

            {

                _textProperty = value;

                PropertyChanged.Notify(() => TextProperty);

            }

        }

We cache the lambda expression:

 

   public class MyClass : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;

        private string _textProperty;

        public MyClass()
        {
            _textPropExpression = () => TextProperty;
        }

        Expression<Func<object>> _textPropExpression;

        public string TextProperty
        {
            get { return _textProperty; }
            set
            {
                _textProperty = value;
                PropertyChanged.Notify(_textPropExpression);
            }
        }
    }

 

I think a 2x overhead would be reasonable if a way could be found to cache the expression and still provide a reasonable API.

 

 

 

 

Jul 20, 2009 at 7:41 PM
Edited Jul 20, 2009 at 7:44 PM

How's about the following (which **seems** from quick tests) to be performing equivalent to regular use of string literals.  Here we use an additional member variable to cache a PropertyChangedEventArgs object which is used thereafter for each change notification for it's associated property.

There is of course a perf hit on the first call, after that it's as quick (actually very marginally quicker - since we're not creating new PropertyChangedEventArgs objects).  There's a slight code cost as well in the couple of extra lines (the var declaration and the caching code).

It seems like a reasonable compromise though - no string literals so it's safe for refactoring tools, and in obfuscation safe.

Code snippet:

    public static class PropChangeHelper
    {
      public static PropertyChangedEventArgs CreateArgs(Expression<Func<object>> propertyExpression)
        {
            var lambda = propertyExpression as LambdaExpression;
            MemberExpression memberExpression;
            if (lambda.Body is UnaryExpression)
            {
                var unaryExpression = lambda.Body as UnaryExpression;
                memberExpression = unaryExpression.Operand as MemberExpression;
            }
            else
            {
                memberExpression = lambda.Body as MemberExpression;
            }
            var propertyInfo = memberExpression.Member as PropertyInfo;
            return new PropertyChangedEventArgs(propertyInfo.Name);
        }
    
    public static class PropChangeHelper
    {
        public static PropertyChangedEventArgs CreateArgs(Expression<Func<object>> propertyExpression)
        {
            var lambda = propertyExpression as LambdaExpression;
            MemberExpression memberExpression;
            if (lambda.Body is UnaryExpression)
            {
                var unaryExpression = lambda.Body as UnaryExpression;
                memberExpression = unaryExpression.Operand as MemberExpression;
            }
            else
            {
                memberExpression = lambda.Body as MemberExpression;
            }

            var propertyInfo = memberExpression.Member as PropertyInfo;

            return new PropertyChangedEventArgs(propertyInfo.Name);
        }
    }

    public class MyClass : INotifyPropertyChanged
    {

        // .... rest of code snipped

        PropertyChangedEventArgs _textPropertyChanged;
        public string TextProperty
        {
            get { return _textProperty; }
            set
            {
                _textProperty = value;

                if(_textPropertyChanged == null) _textPropertyChanged = PropChangeHelper.CreateArgs(() => TextProperty);

                PropertyChanged(this, _textPropertyChanged);
            }
        }

What do you think?

 

p.s. come to think of it, there's probably no reason we can't make the cached event args reference static, and usually only take a perf hit once per class rather than per instance.

 

Jul 30, 2009 at 3:13 PM

Josh, I've posted the following suggestion to Sacha Barber as well...

I think this is now a pretty decent solution to use of static reflection (lamba expressions) for property change notifications which doesn't have a performance hit.

In fact, according to my rough tests it's approx 40% faster than the usual practice of newing up a PropertyChangedEventArgs object with a string for each change notification.

Here's the code (Observable) plus an example usage (Person):

public class Observable<T> : INotifyPropertyChanged
        where T : Observable<T>
    {
        public event PropertyChangedEventHandler PropertyChanged;

        protected static PropertyChangedEventArgs CreateArgs(
            Expression<Func<T, object>> propertyExpression)
        {
            var lambda = propertyExpression as LambdaExpression;
            MemberExpression memberExpression;
            if (lambda.Body is UnaryExpression)
            {
                var unaryExpression = lambda.Body as UnaryExpression;
                memberExpression = unaryExpression.Operand as MemberExpression;
            }
            else
            {
                memberExpression = lambda.Body as MemberExpression;
            }

            var propertyInfo = memberExpression.Member as PropertyInfo;

            return new PropertyChangedEventArgs(propertyInfo.Name);
        }

        protected void NotifyChange(PropertyChangedEventArgs args)
        {
            if (PropertyChanged != null)
            {
                PropertyChanged(this, args);
            }
        }
    }

    public class Person : Observable<Person>
    {
        // property change event arg objects
        static PropertyChangedEventArgs _firstNameChangeArgs = CreateArgs(x => x.FirstName);
        static PropertyChangedEventArgs _lastNameChangeArgs = CreateArgs(x => x.LastName);

        string _firstName;
        string _lastName;

        public string FirstName
        {
            get { return _firstName; }
            set
            {
                _firstName = value;
                NotifyChange(_firstNameChangeArgs);
            }
        }

        public string LastName
        {
            get { return _lastName; }
            set
            {
                _lastName = value;
                NotifyChange(_lastNameChangeArgs);
            }
        }
    }

 

 

Coordinator
Jul 30, 2009 at 4:39 PM

That's pretty nice!  Aside from the fact that the subclasses must manage event argument instances, I think that's a nice way to leverage the compile-time safety of expressions without the performance loss.  I'll think about adding a new overload of RaisePropertyChanged to ObservableObject that takes in the event arg...Thanks!

Jul 31, 2009 at 12:10 AM

Cool! Thanks!

Josh, note, and I think you'll already have though of this, but.. think it will still be desirable for there to be a call into your VerifyPropertyName method for the debug build - there's still after all the possibility of the developer using this passing in the wrong event args object.

And means a lot to me that you like any piece of code of mine - no matter how small - I've been reading a lot of your MVVM and WPF articles and you've been a great help to me as a result :)

Phil

 

Coordinator
Jul 31, 2009 at 4:59 PM

I'm not sure there's a need for verifying the property name if the user is passing in the eventargs.  VerifyPropertyName() only ensures that the specified name corresponds to a public property on the object...it does not ensure that the property that is being set matches the name of said property.  Also, sometimes when setting a property there is a need to raise PropertyChanged for other properties whose values are dependent upon the value of the property being set. In that case, we wouldn't want VerifyPropertyName() to consider that to be erroneous.

Jul 31, 2009 at 9:33 PM

Ah, gotcha - i thought you were walking the callstack to check the property name.  In any case you're right, it wouldn't be a good idea for the reason you describe.

Phil

 

Aug 6, 2009 at 2:33 PM

A quick note, on the code snippet I posted Josh - there's a problem with it - the CreateArgs method is constrained to T which is specified by the inheriting class as itself.

I did this in an attempt to avoid having to have the user of the method explicitly specify the type of T, which otherwise it has to do since it is used from static scope.

Unfortunately I think this isn't going to work since it constrains T to the immediate subtype. This is all well and good if that's the final class and there aren't any further levels of inheritance.

So, for example if we were to subclass Person, as it stands with the posted code, the subclass would not be able to create property change arg objects for it's own properties, only those of Person.

I'm on vacation right now, so I'll take another look at it later, but the most obvious way to resolve this would seem to be to make CreateArgs a generic method, and have the caller explictly state the type for T. We can also possibly remove the base class Observable, but should at least make it non-generic since specialization on T for the base class no longer serves any purpose.

Regards,
Phil

Coordinator
Aug 6, 2009 at 3:54 PM

Thanks Phil.  I was against making ObservableObject generic, so that was never on my radar.  Thanks for pointing out that problem, though.  It's another reason to not make it generic.  :)

Josh