Using reflection for GUIControlFactory (1 Viewer)

A

Anonymous

Guest
I've been going over the source, trying to see where I can contribute and came across GUIControlFactory.cs.

Is there any reason that reflection isn't used to set the properties on the controls? This would make maintenance of this class trivial since a piece of generic code could be written (using reflection) that would set the properties on the controls as xml nodes are encountered. This adds some restrictions on the xml format (the node names must match the property names) but this would make adding properties and new controls relatively trivial rather than special casing the code for each control.

To do this will require a slight change to the control objects as they'll all need empty constructors and an init function to take the place of the code that was in the current constructors.

Doing this will also enable the ability to log nodes in the xml that don't actually pertain to a property on the control, eventually a DTD could be created based off of the object definitions themselves.

Any comments?

-Suicidal
 

barranger

Portal Pro
May 31, 2004
67
0
Sounds great, but I'm wondering if the relfection would add overhead to the initial startup of MP which is borderline too long as it is.

Just a thought,
Domi_fan
 
A

Anonymous

Guest
If C#'s reflection performs anywhere close to Java's then it shouldn't be much of a performance hit. ;

MP's load time isn't too bad when you compare it to Beyond TV3, although MP could probably benefit from loading some of the windows in a background thread rather than loading them all upfront (I think that's what its doing).

-Suicidal
 

barranger

Portal Pro
May 31, 2004
67
0
True there is a lazy load option I think in MP but I'm not sure it's ever used. Also, just cause BeyondTV is worse, doesn't mean we should be lax on this.

Just a thought,
Barranger
 

NoMoDo

Portal Member
September 9, 2004
23
0
Israel
Reflection in MP

@suicidal - Thanks to you, I've come up with an interesting enhancement to the GUIWindow class, that uses Reflection to make plugin development and skin development easier (see below). I agree with you 100% - a similar change to the ControlFactory class is sorely needed - the class itself and especially the foot-long Create method could really use a big change to improve maintainability.

A Reflective GUIWindow (or: There's more than one way to skin a cat)

Purpose: To make skinning in MP easie for both programmers and skin
designers.
The idea is to have developers declare the controls on their GUIWindows as field members (as in Windows Forms), which will then be initialized automatically when the window is loaded. This is achieved through the use Reflection and Attributes.

Advantages:
  • 1. Automatic generation of the XML skin template, which you could then
    immediatly put to use with the forthcoming Skin Editor or a regular XML editor.
  • 2.Eliminates the use of
    Code:
    enum Controls { ... }
    which clutters up the code considerably, and the cumbersome repeated calls to GUIWindow.GetControls(...). Instead, using my method, controls
    on the window are fields of the window's class, making for a much better, more intuitive, and easier design process. (The exact type of each GUIControl is defined inside the class).
  • 3.The "description"and "label" tags of every control are hardcoded into
    the GUIWindow descendant class, rather than being tucked away
    in the skin's XML data.

ToDo: Add chapter to the "How to create plugins tutorial" once changes are agreed upon and in CVS. Also: decide on a coding convention for controls (perhaps c_refreshButton, or frodo's m_pRefreshButton).

I've uploaded my patch to SourceForge.
The three files that should be added to guilib in order to make this work are SkinnableWindowAttribute, ControlAttribute, and the changed GUIWindow. Also included in the patch file are an example mock-up window class "My Lyrics" to demonstrate the usefulness of my changes, and a demonstration application project that takes an assembly such as the GUILyrics assembly (or any assembly that has a GUIWindow marked with the new attributes) and generates a XML skin template.
 

NoMoDo

Portal Member
September 9, 2004
23
0
Israel
Weeeeeeeeee.

@Suicidal - Thank you for your wise and insightful suggestion.
I've decided it to do it myself, and worked frantically over the last three days.
My implementation is about 96% done. In fact, I have MP loading using the re-written-from-scratch GUIControlFactory. If I'm not just insanely sleep deprived and imagining things, I think this change of architecture has brought us some significant performence improvements (NProf thinks so too, but I should really do more testing to be sure).

The way I did it was to simply let GUIControlFactory set the values on the private fields on the control. Each of these private fields was maked with a "XMLSkinElementAttribute", which contains the name of the corresponding element in the xml.

More about the performence aspect of this change: My testing shows that using reflection to set a value on a private field of a class is only about 6 to 8 times longer than a regular assignment to a private field member. Of course, there's the additional cost of reflecting the GUIControl class to find all of it's skinnable fields, but that is only done once per control (using a Hashtable as a cache), so that cost is totally insignificant.

I'd also like to stress that while this may seem like a huge change that might introduce problems, I did do my part to try and make sure nothing goes wrong - as I changed the controls incrementally to use my "new way", I made sure they can still be created by the "old" GUIControlFactory - and ran MediaPortal and made it create two instances, one using old Factory class and one using the new one, and used a class I wrote called "InstanceComparer" that logged any differences between the two. If there were any, I fixed my changes. So I guess you could say I did my part for software testing.

I think I'll have my patch up on sourceforge by tomorrow.
 
A

Anonymous

Guest
Don't know what your talking about, but it sounds great. Thanks NoDoMO for this huge effort. Just wanted you to know that everyone really appreciates your hard work.
 

JoeDalton

Retired Team Member
  • Premium Supporter
  • September 27, 2004
    425
    18
    55
    Belgium
    Home Country
    Belgium Belgium
    According to this article using reflection for setting private variables is 40 times slower than using some kind of SetVariable(string variableName, object value) method.

    I can see a number of reasons why your "reflection-version" performs better, without looking at the code.

    For example take the following function:

    bool GetString(ref XmlNode RootNode, string strTag, ref string strStringValue)
    {
    XmlNode node = RootNode.SelectSingleNode(strTag);
    if (node == null) return false;
    if (node.InnerText == null) return false;
    if (node.InnerText == "") return false;
    strStringValue = node.InnerText;
    return true;
    }

    There is abolutely no reason why the XmlNode should be passed as a ref argument because its instance is not changed in this function. You should only use ref arguments if you want to assign another instance to it in the function like it is the case with the string value.

    [technical stuff]
    In C# all objects are automatically passed by ref. What is done here is passing a pointer to the pointer of the object instead of passing just a pointer to the object.

    This gives more work to the CLR, because now it has to check whether the pointer that is passed points to the correct type of object
    [/technical stuff]

    Second
    a lot of casting is going on in that class this also takes a lot of time.
    For example, instead of this:
    if (strType == "image")
    {
    strTexture = ((GUIImage)reference).FileName;
    dwColorKey = ((GUIImage)reference).ColorKey;
    bAspectRatio =((GUIImage)reference).KeepAspectRatio;
    bFiltered =((GUIImage)reference).Filtering;
    bCentered =((GUIImage)reference).Centered;
    }

    5 casts!!!

    one should write this:
    if (strType == "image")
    {
    GUIImage img = (GUIImage)reference;
    strTexture = img.FileName;
    dwColorKey = img.ColorKey;
    bAspectRatio = img.KeepAspectRatio;
    bFiltered = img.Filtering;
    bCentered =img..Centered;
    }

    1 cast


    I guess a lot of performance improvements are possibe in this class...


    [/url]
     

    Frodo

    Retired Team Member
  • Premium Supporter
  • April 22, 2004
    1,518
    121
    52
    The Netherlands
    Home Country
    Netherlands Netherlands
    This is all nice,
    but when you discuss performance you should make a clear difference in
    1. startup/loading time
    2. cpu% when MP is up & running

    Seconly you should profile an application first to see where exactly the bottlenecks are
    We're looking at GUIControlFactory.cs now and the suggestion is made that this causes 99% of all loading time.
    But did anyone check this? Maybe its the jitting which takes up much time, or loading the images or,......

    Second this GUIControlFactory.cs is only used for startup
    when MP is running GUIControlFactory.cs is not doing anything
    (so its only relevant for point 1. not for point 2)

    I did extensive profiling for 2. and there almost all cpu% is spent in
    guilib/GUIFont.cs to draw text. Already i made some performance enhancements to it, but thats still the biggest bottleneck for point 2.

    I didnt do any profiling for point 1. yet. But i suggest todo this first
    before speculating that a cast or ... is responsable for all the loading time


    Frodo
     

    NoMoDo

    Portal Member
    September 9, 2004
    23
    0
    Israel
    Sorry, I didn't make myself clear.

    First, I am referring of course only to #1, startup\loading time.
    Second, I did do profiling, I wasn't speculating, I just wasn't sure if I should rely on only one profiling tool.

    As I mentioned in my first post, I used NProf, which seems to be a very thorough and accurate tool for measuring the relative duration the program spends in each of function. It starts measuring when you start the program, and stops when you exit. To profile the change to GUIControlFactory, I profiled only by opening MP, and then immediately closing it when the Home plugin appeared.

    I think it's also a fair speculation that the creation of many many short-term GUI reference controls took its toll on the GC (my method doesn't make any reference controls - it keeps the reference nodes themselves in a Hashtable instead and updates the reference data to the controls directly.)

    Now, here are the actual numbers from NProf (For loading MP with Metal skin, and closing as soon as Home appears):
    The results presented here are from a single test (not averages), but I've conduct the test quite a few times before and got (more or less) the same numbers.

    Before (With old GUIControlFactory)
    Calls to GUIWindow::LoadSkin(): 15
    % of total execution time in all threads: 43.72%

    in which ->

    Calls to GUIControlFactory::Create: 228
    % of total execution time in all threads: 37.54%

    in which ->

    Calls to GUIControlFactory::GetString: 15190
    % of total execution time in all threads: 17.99%

    Calls to GUIControlFactory::GetInt: 12495
    % of total execution time in all threads: 14.50%

    etc...



    After (with the rewritten GUIContrlFactory
    Calls to GUIWindow::LoadSkin(): 16
    % of total execution time in all threads: 7.74%

    in which ->

    Calls to GUIControlFactory::Create: 228
    % of total execution time in all threads: 2.22%



    Analysis:
    As we can see the GUIControlFactory class is responsible for approximatly 37% of the loading time in MP, creating a serious bottleneck. The main reason for this performance bottleneck was tens of thousands of unnecessary calls to the various Get functions which attempted to retrieve data from the XML node. My method eliminates this problem by stepping over the xml nodes and retrieving valid data where it can be found, so the maximum amount of calls to the data retrieval function is the number of valid elements in the reference control + the number of valid elements in the control node itself. This way, checking wether the data is actually there is also unnecessary.
     

    Users who are viewing this thread

    Top Bottom