Yet a couple of other performance suggestions (1 Viewer)

[ojo]

Hi again,

recently i "attended" a streamed webcast from micrsoft .NET shop. called Code Optimization from September 10. 2002.

In here there talking about how the GC works and what is do'es and don'ts for programming .net.

To my suprise you can hear Gregor Noriskin (Performance Program manager for the CLR team at Microsoft) state that this code construct is fairly expensive

      int id=0;

      // al being a collection of ints. In this case an ArrayLists of ints
      foreach (int i in al)  
      {
        // some computation using 
        id = i;
      }

This is quite a nice looking approach to having fairly self documenting code but it turns out that the CLR is far better optimized for executing this code:

      int id=0;
      int c = al.Count;

      for (int i=0; i < c; i++)  
      {
        id = (int) al[i];
      }

In essense the same semantics with a different syntax. But a huge difference. It turns out that the difference is at least a factor 2. Foreach is takes doubble the time to execute in comparison with the old for style looping. And this regardless of the count of elements in the collection.

This might not seem like a lot, but looking at the MP code today at least 124 source code files contains one or multiple foreach'es some of them might even be nested.

I know this could turn out to be nearly not measureable, but it's something to take into account.

Who knows changing them all might give stunning results !!!!

Interested check the cast here: http://msdn.microsoft.com/theshow/Episode027/default.asp

The last thing i'd like to point out is the Splash.cs. From main the constructor calls both the SetInformation and SetVersion methods. Both of these Yields an Update (redraw). And the constructor itself is only called from Main in this construct:

      splashScreen = new SplashScreen();
      splashScreen.SetVersion(clientInfo .InstalledVersion);
      splashScreen.Show();
      splashScreen.Update();

To prove my point I've tried to write down the execution sequence here:

      splashScreen = new SplashScreen();
      // splash constructor
        ...
        SetInformation("Loading...");
        // Yields an Update()

        SetVersion(Application.ProductVersion);
        // Yields an Update()

      // end splash constructor

      splashScreen.SetVersion(clientInfo .InstalledVersion);
      // Updates the version again and 
      // Yields yet another Update()

      splashScreen.Show();

      // The only Update needed as far as I can tell
      splashScreen.Update();

This isn't much but it's some...

Happy coding,

Ojo

 

[Frodo]

I was already aware that the foreach(...) is a performance killer

therefore you'll notice that the foreach(...) calls are not used in routines
which get called every frame
like Render()
or FrameMove()

I only use them for things which happen not very often.

Frodo

 

[ojo]

Sorry I didn't check that.

Its a tradeoff. Do You wan't nice code or faster execution. I think you found the correct solution for the paradox. Use for-loops when performance really matters, but use foreach when it don't.

Like i said earlier, nice going. :wink:

I'we looked on the AMS-XML stuff. Seems to me that starting some of the methods gets called over 200 times for the same 30 or so values in MediaPortal.xml. Several of the times the class is even re-reading the file from disc.

Maybe I can come up with a solution to at least keep the re-reading from happening during MP-startup.

Ojo

 

[Frodo]

yeah the settings stuff can prolly be improved
luckily the settigns are not re-read 1000x a second
Most of the times settings are re-read when you
-switch to another screen
-play a movie
-switch tv channels
...

so on user actions

frodo

 

[ojo]

yeah the settings stuff can prolly be improved

What do You thing of the following solution:

Make AMS.Profile.XML a Singleton, created only-once and the same instance getting reused all over the code.

The Contructor should read the file into interal XMDocument structure m_doc once and for all.

But setting up a FileWatcher on the file to make sure when the file eventually gets changed the file will be re-read into m_doc. But only then. Until that occurs every "instantiation" would be changed to a getInstance and every read-operation would therefore be converted to a memory only operation.

Internally a good part of the check for file is read and so-on would be removed optimizing the code further.

But the overhead would be registering an event for the FileWatcher stuff. But since this is a "system-routine" I'd expect this to be fairly cheap.

I'll be happy to supply some code, but I would like Your comments before changing the code.

Ojo

 

[ojo]

yeah the settings stuff can prolly be improved

I did some profiling using the newly released version of NProf (0.0.9) check http://nprof.sourceforge.net (they seem to have some problems updating the sourceforge pages, so you will have to check this page for the newest download http://sourceforge.net/project/showfiles.php?group_id=74129)

I found out that in the "main thread" 28% of the processing were done inside AMS.Profile.GetXmlDocument. 59 times the method was called and everytime the document was re-read, reparsed by the System.XML validator and populated into a XmlDocument object.

So i did some thinking. Why not read it once and just "cache" it. I realize this is not the final solution, since you will have to make even further changes to make sure the save method works correctly and maybe implement a FileWatcher to get it right. But it is "prove of concept" for the Singleton idear.

I made an new Class called SingletonXmlHolder. This class ensures that it only initializes once and reads the XML document once. Then I changed a bit in the GetXmlDocument() method and the result (drumrolls......) 0,15% of the processing time.

namespace AMS.Profile
{

  class SingletonXmlHolder
  {
    private static SingletonXmlHolder instance;
    private static System.Xml.XmlDocument _XmlDocument = new System.Xml.XmlDocument();

    private SingletonXmlHolder()
    {
    }  

    public static SingletonXmlHolder GetInstance(string m_strFileName)
    {
      if(instance == null)
      {
        instance = new SingletonXmlHolder();
        _XmlDocument.Load(m_strFileName);
      }
      return instance;
    }            

    public System.Xml.XmlDocument XmlDocument
    {
      get { return _XmlDocument; }
    }
  }  

  public class Xml :IDisposable
  {
    // Fields
    private string m_rootName = "profile";

.....

    private XmlDocument GetXmlDocument()
    {
      // Check kept for the moment but should be moved to SingletonXmlHolder
      if (!File.Exists(m_strFileName))
                 return null;

      SingletonXmlHolder snglXmlHolder = SingletonXmlHolder.GetInstance(m_strFileName);
						
      return snglXmlHolder.XmlDocument;
    }

.....
}

What do you think ?

Ojo

 

[Anonymous]

yeah the settings stuff can prolly be improved

I did some profiling using the newly released version of NProf (0.0.9) check http://nprof.sourceforge.net (they seem to have some problems updating the sourceforge pages, so you will have to check this page for the newest download http://sourceforge.net/project/showfiles.php?group_id=74129)

I found out that in the "main thread" 28% of the processing were done inside AMS.Profile.GetXmlDocument. 59 times the method was called and everytime the document was re-read, reparsed by the System.XML validator and populated into a XmlDocument object.

So i did some thinking. Why not read it once and just "cache" it. I realize this is not the final solution, since you will have to make even further changes to make sure the save method works correctly and maybe implement a FileWatcher to get it right. But it is "prove of concept" for the Singleton idear.

I made an new Class called SingletonXmlHolder. This class ensures that it only initializes once and reads the XML document once. Then I changed a bit in the GetXmlDocument() method and the result (drumrolls......) 0,15% of the processing time.

namespace AMS.Profile
{

  class SingletonXmlHolder
  {
    private static SingletonXmlHolder instance;
    private static System.Xml.XmlDocument _XmlDocument = new System.Xml.XmlDocument();

    private SingletonXmlHolder()
    {
    }  

    public static SingletonXmlHolder GetInstance(string m_strFileName)
    {
      if(instance == null)
      {
        instance = new SingletonXmlHolder();
        _XmlDocument.Load(m_strFileName);
      }
      return instance;
    }            

    public System.Xml.XmlDocument XmlDocument
    {
      get { return _XmlDocument; }
    }
  }  

  public class Xml :IDisposable
  {
    // Fields
    private string m_rootName = "profile";

.....

    private XmlDocument GetXmlDocument()
    {
      // Check kept for the moment but should be moved to SingletonXmlHolder
      if (!File.Exists(m_strFileName))
                 return null;

      SingletonXmlHolder snglXmlHolder = SingletonXmlHolder.GetInstance(m_strFileName);
						
      return snglXmlHolder.XmlDocument;
    }

.....
}

What do you think ?

Ojo

I think people like you are rare, man you really post interesting stuff... again this is my opinion as a non programmer

People like you are needed.

 

[ojo]

I think people like you are rare, man you really post interesting stuff... again this is my opinion as a non programmer

People like you are needed.

Thanx :lol: glad You think so! Hopefully "frodo" find this info just as usefull !

Ojo

 

[FlipGer]

Hi ojo,

just wanna say: WOW! 8) Thanx for those tips. It is nice to have you here. I am quite sure frodo is happy to get these infos.

So keep on going.

Flip.

 

[ojo]

After the general idear having XML nodes cached into a HashTable and proving at least a factor 10 performance optimization, I would suggest rewriting XML.cs even futher. (See other post "profiling for performance")

Instead of just caching the XML file into a XMLDocument using a Singleton pattern, it would be better to cache the document into a Hashtable, thereby optimizing both object creation and access to entries. This could bedone since the MediaPortal.XML never exceeds 2 levels of xml-nodes (besided the root-node). Just cache the elements with the key as <<section>>\<<entry>>.

Having the MediaPortal.xml cached also means that the idear of FileWatcher looking for changes done from the configuration or by hand also must be implemented.

I expect this to cut down memory-usage and garbage collections during load quite considerable. It will also speed up performance during load, but "only" about further 4-5 percent.

Just to be speciffic. This will only reduce load time of MP, not overall performance running the app.

Ojo