Lazy Performance When I want to cancel

Lazy Performance When I want to cancel

Old forum URL: forums.lhotka.net/forums/t/693.aspx

JoOfMetL posted on Friday, July 21, 2006

I Download the Project Tracker, and I wanted to test it whit a lot of data.

I add a project with 500 ressources.

And I Click on the button Cancel.

The application is frozen during a very long time (P4 XEON 3 Ghz, 1 GO RAM)

I think the problem is : The Event OnPropertyChanged is called too many time.

Why  the Event OnPropertyChanged is called foreach properties and not one time after the object reloaded.

Do you have a solution to my problem ?

It's a serious problem for my futur application.

Thank you.

tetranz replied on Friday, July 21, 2006

I have struggled with the same thing. It is definitely a real issue. I think you get a PropertyChanged event and therefore also a ListChanged event for every property of every item in the collection so the UI really gets hammered with events.

Setting RaiseListChangedEvents = false on your collection will be some help but I found that it really wasn't enough. That stops the UI being hammered but there is still a lot of unnecessary processing happening with your collection receiving PropertyChanged events.

I made a simple modification to CSLA to give me a property called INotifyPropertyChangedEnabled. It defaults to true so I can usually ignore it.. It lets me stop PropertyChangedEvents when doing a cancel. This makes a huge difference. It means that you need to refresh the databinding after the cancel.

Perhaps Rocky could consider doing something more formal about this in a future release of the framework.

Cheers
Ross

JoOfMetL replied on Friday, July 21, 2006

Thank you for your response.

I try an another solution to solve the problem to Cancel.

I modified the framework :

 [Serializable()]
  public abstract class BusinessBase :
    Csla.Core.UndoableBase, IEditableBusinessObject,
    System.ComponentModel.IEditableObject, System.ComponentModel.IDataErrorInfo,
    ICloneable
  {
 ....

public void CancelEdit()
    {
      UndoChanges();
      this.OnPropertyChanged("TEST");          <------------
    }


 protected override void UndoChangesComplete()
    {
      _bindingEdit = false;
      ValidationRules.SetTarget(this);
      AddBusinessRules();
      //OnUnknownPropertyChanged();      <-------------
      base.UndoChangesComplete();
    }

It seems that it's works.

What do you think about this ?


RockfordLhotka replied on Friday, July 21, 2006

This has been discussed in another thread as well.

The issue is one I discuss in the book - OnUnknownPropertyChanged. As I point out in the book, the default implementation is not ideal. And so I made the method Overridable/virtual so you can create a more efficient implementation.

If you look, you'll see that it raises PropertyChanged for every property. This is because it MUST raise the event for a property that is actually bound to the UI, and there's no way for the object to know what properties are and aren't bound to any given UI.

But YOU might know. You might know that a particular object always has its Name property, or Id property or whatever, bound to a control. You can override this method to raise only that one PropertyChanged event - thus avoiding a LOT of overhead.

As was discussed in the other thread, you could use this technique to recreate the hack I used in .NET 1.x - where I always raised the equivalent of PropertyChanged("IsDirty"). Then you just make sure to always bind the IsDirty property to a CheckBox control with size 0,0 on every form.

You can do this globally by creating your own base class that sits between BusinessBase<T> and your actual business objects (which is probably a best practice anyway, because there are other things you may want to customize with such an intermediate base class).

JoOfMetL replied on Friday, July 21, 2006

Thank you for your response. (I read your book and I find it very good !)

But I don't understand why you said :

"This is because it MUST raise the event for a property that is actually bound to the UI, and there's no way for the object to know what properties are and aren't bound to any given UI."

I think that if you raise the PropertyChanged Event with PropertyChangedEventArgs = null
the UI rebind all properties.

 public event PropertyChangedEventHandler PropertyChanged;

private void OnPropertyChanged() {
            if (PropertyChanged != null) {
                PropertyChanged(this, null);
            }
        }

MSDN : The PropertyChanged event can indicate that all the properties of the object were modified using Null reference (Nothing in Visual BASIC) or of String.Empty like name of property in PropertyChangedEventArgs.

Thank you

RockfordLhotka replied on Friday, July 21, 2006

See, you learn something new every day Smile [:)]

You can test this by overriding OnUnknownPropertyChanged and just calling OnPropertyChanged(""). If you find that to work please post here and I can change the framework itself accordingly - probably for version 2.1.

I hope that really does work, as it would sure simplify this issue rather a lot!

JoOfMetL replied on Monday, July 24, 2006


I modified the Framework :

    protected virtual void OnUnknownPropertyChanged()
    {
        //PropertyInfo[] properties =
        //  this.GetType().GetProperties(
        //  BindingFlags.Public | BindingFlags.Instance);
        //foreach (PropertyInfo item in properties) {
        //    OnPropertyChanged(item.Name);
        //}
        OnPropertyChanged(string.Empty);
    }

and it seems that it works.

But I test with One Project and 500 ressources assigned. With this method the Event OnPropertyChanged is raise 501, so it's not optimized.

I Purpose :

    public void CancelEdit()
    {
      UndoChanges();
      OnUnknownPropertyChanged();  <-------
    }

    protected override void UndoChangesComplete()
    {
      _bindingEdit = false;
      ValidationRules.SetTarget(this);
      AddBusinessRules();
      // OnUnknownPropertyChanged();     <--------------
      base.UndoChangesComplete();
    }

So the event OnPropertyChanged is just raise one time.

What do you think about this ?

Thank you.

xal replied on Monday, July 24, 2006

Well, If you have 500 child objects it's reasonable for each of them to fire that event.

Generally, when I have a potentially huge collection I mark it as NotUndoable(), because I will not likely want to save undo data for thousands of items when the user is not likely to edit more than just a few.
The problem is not only storing the data in memory, you also need time to process that.

Also, another question you need to ask yourself is: How necesary is it to call CancelEdit in your forms when you close them? There are few situations where you're actually going to be using the same object in another form, so in general when you close a form you could just forget about the bo, not call CancelEdit() and let the GC take care of it...

I realize that purists will say it's not the best approach, but it does get the job done.

Andrés

JoOfMetL replied on Monday, July 24, 2006


To make my test, I used the tracker project.

I created a project having 500 assigned resources. even if it doesn't seem real, there must a case it occurs.

To my mind I don't intend to call the cancel method inside the close

Thank you for your answer

RockfordLhotka replied on Monday, July 24, 2006

JoOfMetL,
 
Thank you for checking on the use of an empty string - that's excellent news! I will most likely change version 2.1 to use this technique.
 
Regarding you other suggestion, I don't believe that is workable in all cases.
 
There could be a case where someone (form, observer, etc.) is bound to a child object. If you do not raise a PropertyChanged event for the child object, then neither the child object, nor its containing collection, will raise their changed events.
 
While your main form would know that the top-level object changed, nothing listening to your child collections or objects would have any clue that their data source has changed.
 
The point being, I believe those 501 events are actually useful and necessary to be complete, because you don't know what other listeners might exist. Well, YOU might, within your app, but the general "you" of all app developers out there can't know :)
 
Rocky
 

JoOfMetL replied on Monday, July 24, 2006


Thank you.

Indeed, it's doesn't work in all cases.

Example :

      -Projet (Parent)
          --Ressoures (Collection of Child)


On my form I Bound a textbox (or a dataGridView) to a field (or all field) on one of the Ressource  object (belonging to the project object)

And I Use this code :

    this.ressourcesBindingSource.DataSource = mProjet.Ressources:

I Agree that if the Parent Object (Projet) raise the event PropertyChanged, the UI isn't refresh.

But, I think, it's more logic to have 2 bindingsource.
      projetbindingsource
      ressourcebindingsource

 with this relation :

            //
            // ressourcesBindingSource
            //
            this.ressourcesBindingSource.DataMember = "Ressources";
            this.ressourcesBindingSource.DataSource = this.projetBindingSource;

and

             this.projetBindingSource.DataSource = mProjet;

And in this case, it's works.

I think of testing like that and if I see that that produced problem under certain conditions I will return to the preceding version

Thank you.
         

JonM replied on Monday, July 24, 2006

Is it possible to embed a simple boolean property that acts a switch in the low-level BO.  You could check the value in the OnUnknownPropertyChanged event and simply not call the PropertyChanged method if it is set to true??  This wouldn't keep the OnUnknownPropertyChanged from being called but you could stop the other events from firing.

JoOfMetL replied on Tuesday, July 25, 2006


Yes it is a good idea. Those which wish to optimize the code could put the boolean at true and the default value would be false.

RockfordLhotka replied on Tuesday, July 25, 2006

Well, it is only a good idea if there are no side-effects. It also has a bad code smell - in other words, it is very clearly a hack.
 
True, sometimes a hack is the only answer - but it should be a last resort, because it will cause me trouble for years to come... I have to justify everything to thousands of people after all :)
 
A better idea might be (and maybe you've tried this?) to tell the bindingsource not to raise events, then tell the bindingsource to CancelEdit, then tell the bindingsource to raise events and refresh.
 
Rocky

From: JoOfMetL [mailto:cslanet@lhotka.net]
Sent: Tuesday, July 25, 2006 2:31 AM
To: rocky@lhotka.net
Subject: Re: [CSLA .NET] Lazy Performance When I want to cancel


Yes it is a good idea. Those which wish to optimize the code could put the boolean at true and the default value would be false.




JoOfMetL replied on Tuesday, July 25, 2006


Yes you are right, a boolean it's not the best solutions.

I try to tell the bindingsource not to raise events, then tell the bindingsource to CancelEdit, then tell the bindingsource to raise events and refresh AND IT'S WORKS.

Thank you.

Copyright (c) Marimer LLC