Mud Designer Code Analysis: Sealed Attributes
I had mentioned a couple of times that I would be refactoring the current engine re-write using the NDepend statically analysis tool. I performed an initial analysis last night and reviewed the report and started to dig in. Some of the issues that I ran in to were really simple to fix. Let's start with those first.
Avoid unsealed attributes
Microsoft's MSDN states that for performance reasons, you should avoid unsealed attributes whenever possible. Considering that the engine's built-in validation mechanics are built off of attributes, I want to make sure and grab any performance increase that I can get.
I started out with an existing Fixture model that I created during development. The model is really simple and includes three properties, each being validated in some manor.
public class ValidatableFixture : ValidatableBase { private const string PasswordConfirmationDelegateName = "ConfirmPasswordsMatch"; public ValidatableFixture() { this.Name = string.Empty; this.Password = string.Empty; this.PasswordConfirmation = string.Empty; } [ValidateValueIsNotNullOrEmpty(ValidationMessageType = typeof(MessageFixture), FailureMessage = "Name must be set.")] public string Name { get; set; } [ValidateValueIsNotNullOrEmpty(ValidationMessageType = typeof(MessageFixture), FailureMessage = "Password must be set.")] [ValidateStringIsGreaterThan(GreaterThanValue = 4, ValidationMessageType = typeof(MessageFixture), FailureMessage = "Password must be greater than 4 characters.")] public string Password { get; set; } [ValidateWithCustomHandler(DelegateName = PasswordConfirmationDelegateName, ValidationMessageType = typeof(MessageFixture), FailureMessage = "Passwords do not match.")] public string PasswordConfirmation { get; set; } [ValidationCustomHandlerDelegate(DelegateName = PasswordConfirmationDelegateName)] public IMessage PasswordConfirmationValidation(IMessage message, PropertyInfo property) { return this.PasswordConfirmation.Equals(this.Password) ? null : message; } }
This fixture contains four total validation rules that will be invoked during the test. The most expensive of the four would be the ValidateWithCustomHandler attribute. Unlike the rest of the validation system, the ValidateWithCustomHandler attribute does not cache the MethodInfo object it fetches via reflection. At the moment, the engine just caches the attributes themselves and their associated properties for re-use across multiple instances. Caching of method delegates will make its way in to the system at some point in the future.
To get started, I wrote a simple unit test that creates 1,000 instances of the ValidatableFixture class and invokes ValidateAll(). The unit test ran through all 1,000 instances, invoking Validate() on each attribute, which caused the attribute validation to take place. In theory, there shouldn't be a huge penalty here due to the caching I am doing. Once the first ValidatableFixture is instanced, the rest of them use the PropertyInfo and ValidationAttribute cache that the first one generated. There won't be any deep walking of the attribute heirarchy.
// Arrange var fixtures = new List<ValidatableFixture data-preserve-html-node="true">(); var watch = new Stopwatch(); for (int index = 0; index < 1000; index++) { var model = new ValidatableFixture(); model.Name = "TestName"; model.Password = "pass"; model.PasswordConfirmation = "pass"; fixtures.Add(model); } // Act watch.Start(); foreach(ValidatableFixture fixture in fixtures) { fixture.ValidateAll(); } watch.Stop(); Debug.WriteLine(watch.Elapsed.TotalMilliseconds);
I ran the test 5 times and ended with an average of 41.3199ms. As you can see, I included the instance set up in the time, since that is where the caching takes place along with the bulk of the GetCustomAttribute< T>() calls.
Finally, I went through and sealed all of the validation classes and re-ran the test. To my surprise, the results ended up be slower than leaving them unsealed, with an average time of 43.3881ms.
I found this to be really interesting, as sealing the class should technically make it faster.
I did some research online and discovered there is actually a debate over whether you should or should not seal your classes. I'm in the camp that they should be sealed unless there is an explicit need to override them. In the case of the validation attributes, they should remain sealed, even at the cost of a bit of performance. The idea behind the validation rules is that developers should write their own implementation of IValidationRule rather than inheriting from an existing one and trying to change one step out of many within the classes validaiton process. It opens up the possibility of unexpected results happening behind the scenes during the validation process.
