Hi folks,
My team is working on a WPF application using CSLA 4.2.1 for our business layer implementation. We have one area within our application that currently displays a list of 5633 info objects. This list is taking approximately 3.0 seconds to load. So, I decided to investigate in order to determine what the bottleneck is. I discovered that the culprit is in the LoadProperty(IPropertyInfo propertyInfo, object newValue) method on ReadOnlyBase<T> that is being called to populate the objects from a data reader.
This particular info object has 33 fields which means that LoadProperty is being called 5633 * 33 or 185,889 times in order to create the list we are trying to display. Upon further inspection, I discovered that this LoadProperty method is doing some pretty heavy lifting via reflection in order to set the field manager properties. I also noticed that a lot of this heavy lifting could be done during type initialization rather than each time a property is loaded.
I wanted to try this idea out to see if it would work and to see whether or not the performance would improve noticeably. What I came up with was this:
public class MyInfoObject : ReadOnlyBase<MyInfoObject>
{
private static readonly Dictionary<IPropertyInfo, System.Reflection.MethodInfo> genericPropertyMethods;
static MyInfoObject()
{
genericPropertyMethods = new Dictionary<IPropertyInfo, MethodInfo>();
var methods = typeof(MyInfoObject).GetMethods(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
var loadPropertyMethod = methods.FirstOrDefault(x => x.Name == "LoadProperty" && x.IsGenericMethod);
var cslaPropertyFields = typeof(MyInfoObject).GetFields(BindingFlags.Static | BindingFlags.NonPublic).Where(x => x.FieldType.GetGenericTypeDefinition().IsAssignableFrom(typeof(Csla.PropertyInfo<>).GetGenericTypeDefinition()));
foreach (var item in cslaPropertyFields)
{
var current = (IPropertyInfo)(item.GetValue(null));
genericPropertyMethods.Add(current, loadPropertyMethod.MakeGenericMethod(new[] { current.Type }));
}
}
protected override void LoadProperty(IPropertyInfo propertyInfo, object newValue)
{
genericPropertyMethods[propertyInfo].Invoke(this, new[] { propertyInfo, newValue });
}
}
The goal here is that instead of creating a generic method to access the property storage every time LoadProperty is called, we create all of the generic methods that will be needed one time at type intialization via a static constructor. We then store those methods in a static dictionary so they can be accessed by all instances of the type. Finally, the LoadProperty method is overridden to look up and invoke methods that are stored in the static dictionary rather than using the base implementation which would run through all the reflection code again.
The results of this experiment were exactly what I expected -- faster performance. The time to load the list was reduced from approximately 3.0 seconds to approximately 1.0 seconds.
This particular scenario was just a test that I setup within our system to gauge performance for loading larger lists. We will have other objects that have roughly the same number of properties that get loaded from the database and may eventually have 10-20x as many records returned once the software is fielded. In the interest of keeping our architecture scalable, we are considering implementing this logic in a generic base class such as this:
public class CustomReadOnlyBase<T> : ReadOnlyBase<T>
{
private static readonly Dictionary<IPropertyInfo, System.Reflection.MethodInfo> genericPropertyMethods;
static CustomReadOnlyBase()
{
genericPropertyMethods = new Dictionary<IPropertyInfo, MethodInfo>();
var methods = typeof(T).GetMethods(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
var loadPropertyMethod = methods.FirstOrDefault(x => x.Name == "LoadProperty" && x.IsGenericMethod);
var cslaPropertyFields = typeof(T).GetFields(BindingFlags.Static | BindingFlags.NonPublic).Where(x => x.FieldType.GetGenericTypeDefinition().IsAssignableFrom(typeof(Csla.PropertyInfo<>).GetGenericTypeDefinition()));
foreach (var item in cslaPropertyFields)
{
var current = (IPropertyInfo)(item.GetValue(null));
genericPropertyMethods.Add(current, loadPropertyMethod.MakeGenericMethod(new[] { current.Type }));
}
}
protected override void LoadProperty(IPropertyInfo propertyInfo, object newValue)
{
genericPropertyMethods[propertyInfo].Invoke(this, new[] { propertyInfo, newValue });
}
}
This would allow all of our business objects to have the same behavior. Because of the way .NET handles static members in generic types, I believe that at run time, each read only business object type, T, would get it's own generic type, CustomReadOnlyBase<T> , which means we would have one static dictionary of methods for each read only business object.
The fact that this implementation is not already in the CSLA ReadOnlyBase<T> makes me question whether or not this is actually a good idea. The performance benefit in my test scenario was obvious, but I'm curious if there are any other implications that I may have overlooked.
So, while this thread was mostly intended to be a discussion, I do still have 2 questions:
1.) Is there a better way to get the performance increase that we are looking for?
2.) Is there a reason the reflection code was being run in every single call to LoadProperty rather than one time for type initialization? Or, to reword this question, is there a reason I should not create a CustomReadOnlyBase<T> with this implementation?
How is your PropertyInfo defined and how do you do data access / loading?
Show us your code!
The non-generic LoadProperty should normally only be called for Properties with private backing field.
So the recommended way is like this:
public static PropertyInfo<int> IdProperty = RegisterProperty<int>(c => c.Id);
public int Id
{
get { return GetProperty(IdProperty); }
private set { LoadProperty(IdProperty, value); }
}
public static PropertyInfo<string> NameProperty = RegisterProperty<string>(c => c.Name);
public string Name
{
get { return GetProperty(NameProperty); }
private set { LoadProperty(NameProperty, value); }
}
private void Child_Fetch(ProjectTracker.Dal.ProjectDto item)
{
Id = item.Id;
/// or
LoadProperty(NameProperty, item.Name);
}
This code will call the generic LoadProperty as the PropertyInfo's are generic.
JohnnyBee,
I'd prefer to not post the code because this is an army project and I don't know exactly what the policy would be for posting source code. That being said, our PropertyInfo fields are defined exactly the same way as what you have in your example. My data access is being done via SQL data reader. Using your example, the code to load the data would look like:
private void Child_Fetch(IDataReader dr)
{
LoadProperty(IdProperty, dr.Item["Id"]);
LoadProperty(NameProperty, dr.Item["Name"]);
}
The reason this is using the non-generic version of LoadProperty is because the value returned by DataReader.Item[ ] is always of type object.
I don't quite understand.
This code:
public class ROTest : ReadOnlyBase<ROTest>
{
public static readonly PropertyInfo<string> S1Property = RegisterProperty<string>(c => c.S1);
public string S1
{
get { return GetProperty(S1Property); }
private set { LoadProperty(S1Property, value); }
}
public ROTest()
{
LoadProperty(S1Property, "csla");
}
}
Csla.dll!Csla.ReadOnlyBase<BusinessRuleDemo.ROTest>.LoadProperty<string>(Csla.PropertyInfo<string> propertyInfo, string newValue) Line 1117 C#
BusinessRuleDemo.exe!BusinessRuleDemo.ROTest.ROTest() Line 20 + 0x19 bytes C#
BusinessRuleDemo.exe!BusinessRuleDemo.Program.Main() Line 20 + 0x15 bytes C#
[External Code]
Calls the generic LoadProperty method and does NOT use Reflection in order to set the property.
But if your S1Property is defined as IPropertyInfo as in this sample:
public class ROTest : ReadOnlyBase<ROTest>
{
public static readonly PropertyInfo<string> S1Property = RegisterProperty<string>(c => c.S1);
public string S1
{
get { return GetProperty(S1Property); }
private set { LoadProperty(S1Property, value); }
}
public ROTest()
{
IPropertyInfo myS1Property = S1Property;
LoadProperty(myS1Property, "csla");
}
}
Csla.dll!Csla.ReadOnlyBase<BusinessRuleDemo.ROTest>.LoadProperty(Csla.Core.IPropertyInfo propertyInfo, object newValue) Line 1165 C#
BusinessRuleDemo.exe!BusinessRuleDemo.ROTest.ROTest() Line 22 + 0x14 bytes C#
BusinessRuleDemo.exe!BusinessRuleDemo.Program.Main() Line 20 + 0x15 bytes C#
[External Code]
the you will hit the generic LoadProperty that uses Reflection.
So please check how your PropertyInfo is declared / passed as parameters!!
The PropertyInfo is definitely declared properly. It's the second parameter that is causing it to use the non-generic overload.
The signature for the generic is:
LoadProperty<P>(PropertyInfo<P> propertyInfo, P newValue)
The signature for the non-generic version is:
LoadProperty(IPropertyInfo propertyInfo, object newValue)
I'll use a modified version of your example:
public static readonly PropertyInfo<string> S1Property = RegisterProperty<string>(c => c.S1);
public string S1
{
get { return GetProperty(S1Property); }
private set { LoadProperty(S1Property, value); }
}public void ROTest()
{
LoadProperty(S1Property, "csla");
}
In this case, the LoadProperty call is overloaded to the generic version because the type parameter, P, is automatically inferred. S1Property is of type PropertyInfo<string> and the string literal "csla" is of course type string as well so everything matches up.
If we change the last method to instead cast "csla" to an object to simulate how the value would be returned from a data reader, the non-generic overload is called.
public void ROTest()
{
LoadProperty(S1Property, (object)"csla");
}
Now, S1property is still OK as PropertyInfo<string> however, the second parameter, newValue, is now of type object so the signature now matches the non-generic version since the type parameter, P, no longer matches the type of the newValue parameter.
That's part of the reason why CSLA offer the SafeDataRader as a wrapper class for IDataReader..
using the SafeDataReader the code would be:
private void Child_Fetch(SafeDataReader dr)
{
LoadProperty(IdProperty, dr.GetInt32("Id"));
LoadProperty(NameProperty, dr.GetString("Name"));
}
And your code would use the generic methods.
I set up a test to try out the SafeDataReader implementation to see what the performance benefit of the generic version of LoadProperty. The 5633 records were returned in 0.76 seconds. The problem with this however is that it does not handle nullable properties very well.
For example, we have several properties on this object that are defined as PropertyInfo<int?>. The default behavior of the generic version of LoadProperty will set the value to 0 when the data reader returns DbNull.Value. We actually need the property to be set to null in this case and not 0. The non-generic version of LoadProperty actually does set the value to null in this case.
That being said, one of my teammates is currently looking into updating our code generator to use the generic version of LoadProperty as well as handle the nullable types. That will be our ideal solution since it gives the best performance benefit.
So far from this experiment I've seen the following results:
Based on these observations, it seems like the generic implementation is still the way to go whenever possible. It also seems like the non-generic implementation that I proposed could be used to significantly reduce the performance gap between the two overloads.
You mentioned that the non-generic implementation of LoadProperty is intended to be used with private backing fields. In one of your other posts here you suggested overriding the non-generic LoadProperty method to set the private backing field. It seems to me if you are using a private backing field, you have no concept of the IPropertyInfo interface -- in fact, you shouldn't need it because it would be simpler to just set the backing field directly. You could create your own implementation to support your private backing fields, but why go through the trouble when you can set them directly and use the generic implementation of LoadProperty to handle all of the managed fields. This leaves me wondering what the actual purpose of the non-generic implementation is in the first place.
Any field that will have an authorization or business rule MUST have a registered PropertyInfo.
So the recommended solution is to have all registered properties an use RelationshipType.PrivateFiekd for those that have private backing fields.
When you use AddOutValue in business rules you must overload LoadProperty to set the Private backing field and most likely also for using ObjectFactory data access with private felds.
Gotcha. That's the part I was missing when I started wondering why the non-generic implementation was needed.
What is the use case for which you require such a huge list to be returned?
These scenarios should typically be handled by server side/ database paging and filtering which is seamless to the user when done properly.
This is the correct answer, IMO. Its unklikely a user is going to shift through over 5000 records, one at a time.
What is the use case for which you require such a huge list to be returned?
These scenarios should typically be handled by server side/ database paging and filtering which is seamless to the user when done properly.
I'm not entirely sure yet whether or not we will actually need to return this many records at once. This thread was the result of an experiment to see if our application could handle that number of records gracefully and so far the answer is no. Right now, I'm simply exploring our options to see how we can improve our performance for returning data from tables that may contain a large number of records.
If we decide that handling the issue at the app server or database level is the way to go, then we may implement that in the future. However, the focus of this thread is the performance of the LoadProperty method and improving this performance will be beneficial regardless of whether or not we decide to handle large data sets server side or client side.
Copyright (c) Marimer LLC