Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TypedPropertyBag to prevent boxing #1319

Closed
GeertvanHorrik opened this issue May 6, 2019 · 21 comments
Closed

Add TypedPropertyBag to prevent boxing #1319

GeertvanHorrik opened this issue May 6, 2019 · 21 comments
Assignees
Milestone

Comments

@GeertvanHorrik
Copy link
Member

@GeertvanHorrik GeertvanHorrik commented May 6, 2019

At the moment all properties are stored in a single (object) dictionary. We should investigate whether it's better to use multiple (static?) dictionaries per type or a single one with boxing.

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented May 6, 2019

Looks like we need boxing when we want to return from a generic method. Here is the current research project:

using System;
using System.Collections.Generic;

namespace CTL1319
{
    class Program
    {
        static void Main(string[] args)
        {
            
        }
    }

    public interface IPropertyBag
    {
        void SetValue<TValue>(string name, TValue value);
        TValue GetValue<TValue>(string name);
    }

    public class NonTypedPropertyBag
    {
        private readonly Dictionary<string, object> _values;

        public void SetValue<TValue>(string name, TValue value)
        {
            _values[name] = value;
        }

        public TValue GetValue<TValue>(string name)
        {
            return (TValue)_values[name];
        }
    }

    public class TypedPropertyBag
    {
        private readonly Dictionary<string, int> _intValues = new Dictionary<string, int>();
        private readonly Dictionary<string, bool> _boolValues = new Dictionary<string, bool>();
        private readonly Dictionary<string, short> _shortValues = new Dictionary<string, short>();
        private readonly Dictionary<string, long> _longValues = new Dictionary<string, long>();
        private readonly Dictionary<string, object> _referenceValues = new Dictionary<string, object>();

        public void SetValue<TValue>(string name, TValue value)
        {
            var targetValue = typeof(TValue);
            if (targetValue == typeof(int))
            {
                SetValue(name, (int)value);
            }
            else if (targetValue == typeof(bool))
            {

            }
            else if (targetValue == typeof(short))
            {

            }
            else if (targetValue == typeof(long))
            {

            }
            else
            {
                SetValue(name, (object)value);
            }
        }

        public void SetValue(string name, int value)
        {
            _intValues[name] = value;
        }

        public void SetValue(string name, bool value)
        {
            _boolValues[name] = value;
        }

        public void SetValue(string name, short value)
        {
            _shortValues[name] = value;
        }

        public void SetValue(string name, long value)
        {
            _longValues[name] = value;
        }

        public void SetValue(string name, object value)
        {
            _referenceValues[name] = value;
        }

        public TValue GetValue<TValue>(string name)
        {
            var targetValue = typeof(TValue);
            if (targetValue == typeof(int))
            {
                return (TValue)GetIntValue(name);
            }
            else if (targetValue == typeof(bool))
            {

            }
            else if (targetValue == typeof(short))
            {

            }
            else if (targetValue == typeof(long))
            {

            }
            else
            {
                return (TValue)GetValue(name);
            }

            return (TValue)_values[name];
        }

        private int GetIntValue(string name)
        {
            return _intValues[name];
        }

        private bool GetBoolValue(string name)
        {
            return _boolValues[name];
        }

        private bool GetShortValue(string name)
        {
            return _boolValues[name];
        }

        private bool GetLongValue(string name)
        {
            return _boolValues[name];
        }
    }

    public class TestType
    {
        private readonly IPropertyBag _propertyBag;

        public TestType(IPropertyBag propertyBag)
        {
            _propertyBag = propertyBag;
        }

        public int IntValue
        {
            get { return _propertyBag.GetValue<int>("int"); }
            set { _propertyBag.SetValue("int", value); }
        }

        public bool BoolValue
        {
            get { return _propertyBag.GetValue<bool>("bool"); }
            set { _propertyBag.SetValue("bool", value); }
        }

        public short ShortValue
        {
            get { return _propertyBag.GetValue<short>("short"); }
            set { _propertyBag.SetValue("short", value); }
        }

        public long LongValue
        {
            get { return _propertyBag.GetValue<long>("long"); }
            set { _propertyBag.SetValue("long", value); }
        }

        public object ReferenceValue
        {
            get { return _propertyBag.GetValue<object>("reference"); }
            set { _propertyBag.SetValue("reference", value); }
        }
    }
}

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Jul 15, 2019

Updated proof of concept:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;
using System.Collections.Generic;

namespace CTL1319
{
    class Program
    {
        static void Main(string[] args)
        {
            var summary = BenchmarkRunner.Run<TypedVsNonTypedPropertyBag>();
        }
    }

    [ClrJob(baseline: true), CoreJob, MonoJob, CoreRtJob]
    [RPlotExporter, RankColumn]
    public class TypedVsNonTypedPropertyBag
    {
        [GlobalSetup]
        public void Setup()
        {
        }

        [Benchmark]
        public int IntGetter_NonTyped()
        {
            var instance = GetTestType(false);
            return GetInt(instance);
        }

        [Benchmark]
        public int IntGetter_Typed()
        {
            var instance = GetTestType(true);
            return GetInt(instance);
        }

        private int GetInt(TestType instance)
        {
            instance.IntValue = 42;

            return instance.IntValue;
        }

        [Benchmark]
        public bool BoolGetter_NonTyped()
        {
            var instance = GetTestType(false);
            return GetBool(instance);
        }

        [Benchmark]
        public bool BoolGetter_Typed()
        {
            var instance = GetTestType(true);
            return GetBool(instance);
        }

        private bool GetBool(TestType instance)
        {
            instance.BoolValue = true;

            return instance.BoolValue;
        }

        private TestType GetTestType(bool typed)
        {
            var propertyBag = typed ? (IPropertyBag)new TypedPropertyBag() : (IPropertyBag)new NonTypedPropertyBag();

            return new TestType(propertyBag);
        }
    }

    public interface IPropertyBag
    {
        void SetValue<TValue>(string name, TValue value);
        TValue GetValue<TValue>(string name);
    }

    public class NonTypedPropertyBag : IPropertyBag
    {
        private readonly Dictionary<string, object> _values = new Dictionary<string, object>();

        public void SetValue<TValue>(string name, TValue value)
        {
            _values[name] = value;
        }

        public TValue GetValue<TValue>(string name)
        {
            return (TValue)_values[name];
        }
    }

    public class TypedPropertyBag : IPropertyBag
    {
        private readonly Dictionary<string, int> _intValues = new Dictionary<string, int>();
        private readonly Dictionary<string, bool> _boolValues = new Dictionary<string, bool>();
        private readonly Dictionary<string, short> _shortValues = new Dictionary<string, short>();
        private readonly Dictionary<string, long> _longValues = new Dictionary<string, long>();
        private readonly Dictionary<string, object> _referenceValues = new Dictionary<string, object>();

        private static readonly Dictionary<Type, object> _setters = new Dictionary<Type, object>();
        private static readonly Dictionary<Type, object> _getters = new Dictionary<Type, object>();

        static TypedPropertyBag()
        {
            _setters[typeof(int)] = (Action<TypedPropertyBag, string, int>)SetIntValue;
            _setters[typeof(bool)] = (Action<TypedPropertyBag, string, bool>)SetBoolValue;
            _setters[typeof(short)] = (Action<TypedPropertyBag, string, short>)SetShortValue;
            _setters[typeof(long)] = (Action<TypedPropertyBag, string, long>)SetLongValue;

            _getters[typeof(int)] = (Func<TypedPropertyBag, string, int>)GetIntValue;
            _getters[typeof(bool)] = (Func<TypedPropertyBag, string, bool>)GetBoolValue;
            _getters[typeof(short)] = (Func<TypedPropertyBag, string, short>)GetShortValue;
            _getters[typeof(long)] = (Func<TypedPropertyBag, string, long>)GetLongValue;
        }

        public void SetValue<TValue>(string name, TValue value)
        {
            var targetValue = typeof(TValue);


            if (_setters.TryGetValue(targetValue, out var setterObj))
            {
                var setter = setterObj as Action<TypedPropertyBag, string, TValue>;
                if (!(setter is null))
                {
                    setter(this, name, value);
                    return;
                }
            }

            // Old-fashioned, potentially boxing, method
            _referenceValues[name] = value;
        }

        private static void SetIntValue(TypedPropertyBag instance, string name, int value)
        {
            instance._intValues[name] = value;
        }

        private static void SetBoolValue(TypedPropertyBag instance, string name, bool value)
        {
            instance._boolValues[name] = value;
        }

        private static void SetShortValue(TypedPropertyBag instance, string name, short value)
        {
            instance._shortValues[name] = value;
        }

        private static void SetLongValue(TypedPropertyBag instance, string name, long value)
        {
            instance._longValues[name] = value;
        }

        public TValue GetValue<TValue>(string name)
        {
            var targetValue = typeof(TValue);

            if (_getters.TryGetValue(targetValue, out var retrievalFuncObj))
            {
                var retrievalFunc = retrievalFuncObj as Func<TypedPropertyBag, string, TValue>;
                if (!(retrievalFunc is null))
                {
                    return retrievalFunc(this, name);
                }
            }

            // Old-fashioned, potentially boxing, method
            return (TValue)_referenceValues[name];
        }

        private static int GetIntValue(TypedPropertyBag instance, string name)
        {
            return instance._intValues[name];
        }

        private static bool GetBoolValue(TypedPropertyBag instance, string name)
        {
            return instance._boolValues[name];
        }

        private static short GetShortValue(TypedPropertyBag instance, string name)
        {
            return instance._shortValues[name];
        }

        private static long GetLongValue(TypedPropertyBag instance, string name)
        {
            return instance._longValues[name];
        }
    }

    public class TestType
    {
        private readonly IPropertyBag _propertyBag;

        public TestType(IPropertyBag propertyBag)
        {
            _propertyBag = propertyBag;
        }

        public int IntValue
        {
            get { return _propertyBag.GetValue<int>("int"); }
            set { _propertyBag.SetValue("int", value); }
        }

        public bool BoolValue
        {
            get { return _propertyBag.GetValue<bool>("bool"); }
            set { _propertyBag.SetValue("bool", value); }
        }

        public short ShortValue
        {
            get { return _propertyBag.GetValue<short>("short"); }
            set { _propertyBag.SetValue("short", value); }
        }

        public long LongValue
        {
            get { return _propertyBag.GetValue<long>("long"); }
            set { _propertyBag.SetValue("long", value); }
        }

        public object ReferenceValue
        {
            get { return _propertyBag.GetValue<object>("reference"); }
            set { _propertyBag.SetValue("reference", value); }
        }
    }
}

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Jul 15, 2019

image

Looks like boxing is 2-3x faster. Will try to get some memory impressions as well.

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Jul 15, 2019

image

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Jul 17, 2019

Closing as this does not seem to provide the benefits I was hoping for.

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 25, 2019

Uploaded the actual PoC to GitHub so it's available for everyone. See https://github.com/GeertvanHorrik/PropertyBagResearch

@GeertvanHorrik GeertvanHorrik added this to the 5.12.0 milestone Oct 25, 2019
@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 18, 2019

@alexfdezsauco did some research, and it looks like the SuperTypedPropertyBag is much better (near same speed, better memory usage):

image

@GeertvanHorrik GeertvanHorrik changed the title Typed property bag dictionaries to prevent boxing Add TypedPropertyBag to prevent boxing Nov 24, 2019
GeertvanHorrik added a commit that referenced this issue Nov 24, 2019
@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 24, 2019

Implemented the property bag here:

https://github.com/Catel/Catel/blob/release/5.12.0/src/Catel.Core/Data/PropertyBags/TypedPropertyBag.storage.cs

@vaecors and @alexfdezsauco , could you please do a code-review / sanity check? The only thing I see that could go wrong is that if a property is set via SetValue, but retrieved via GetValue, it will not work. Maybe add a single dictionary that contains a property name to type dictionary?

@ghost
Copy link

@ghost ghost commented Nov 24, 2019

@GeertvanHorrik On it, will leave feedback here shortly.

@ghost
Copy link

@ghost ghost commented Nov 24, 2019

@GeertvanHorrik So all-in-all the implementation looks good. However one thing really stuck out with me.

For instance these methods:

protected Dictionary<string, Object> GetObjectStorage()

Would be better off being a protected cached property rather than a method. This is a matter more of personal taste I suppose but if you're using the Roslyn FxCop analyzers or SonarCloud their rulesets would probably trigger this as code smell since they're simply returning results from a private backing field, are not virtual and are not calling an external source for data.

I agree with a mapping dictionary with the property name as the key and the type as a value to solve the SetValue/GetValue dilemma as it would only be initialized once anyway and not really impactful to performance.

I'm also concerned about the cyclomatic complexity being high on both GetValue and SetValue but that can't be helped. Even if you switched over to a switch/case instead of if conditions (they'd inline optimize to the same thing anyway) it would still be high.

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 24, 2019

Thanks for the review. Just did some research on Lazy (see https://referencesource.microsoft.com/#mscorlib/system/Lazy.cs,8b99c1f377873554), to make sure it's not taking too much memory. What I want to prevent is the property bag already allocating memory which might not be needed for the bag at all (e.g. how often does one store Char or ushort)? My idea with the methods was to keep this as simple and fast as possible with the lowest possible allocation count.

The only thing that's really allocated for each property bag is the Dictionary<string, Type> with type safety registrations.

@ghost
Copy link

@ghost ghost commented Nov 24, 2019

I probably should have been more specific when I mentioned the use of cached properties. They wouldn't need to necessarily implement Lazy in this case which tends to be the norm these days for cached properties but do it the more direct way before Lazy existed, such as below:

protected Dictionary<string, object> ObjectStorage
{
    get 
    {
        if (_objectStorage is null) 
        {
            _objectStorage = new Dictionary<string, object>();
        }

        return _objectStorage;
    }
}

I usually still do this pattern where I feel the use of Lazy isn't necessary. I usually keep the use of Lazy for cached properties only if their initialization is expensive (such as remote data retrieval).

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 24, 2019

Under the hood, this will result in a GetMethod any way so I think it's semantics at this point? I've updated lots of property bag code tonight and will now run the benchmarks with the actual implementations to see how good it really is.

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 24, 2019

image

Note that this is setting a ton of properties dynamically in each test (so it's not really that slow ;-) ). I will continue running benchmarks, but memory usage of the typed property bags is really good (and note that the PropertyBag already takes full usage of the BoxingCache here).

@ghost
Copy link

@ghost ghost commented Nov 24, 2019

Under the hood, this will result in a GetMethod any way so I think it's semantics at this point? I've updated lots of property bag code tonight and will now run the benchmarks with the actual implementations to see how good it really is.

Understood, like I said in my initial comment it was more of a personal preference deal and a concern about it being flagged as code smell in Sonar. The performance looks promising though.

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 24, 2019

The TypedPropertyBag is a bit slower, but better for memory usage in general. Now the question is: what do we prefer?

Maybe allow injection of the property bag? Not sure what the best way to go is: use PropertyBag or TypedPropertyBag as defaults.

@ghost
Copy link

@ghost ghost commented Nov 24, 2019

That’s where it will get tricky. I think allowing injection would be a good idea to let the implementer choose what which one to use based on their use case. At the same time sane defaults should also be set.

So if I’m not mistaken one of the places the PropertyBag is used in the serializer/deserializer? So maybe a balance there is the untyped for serialization and typed for deserialization would be a pathway.

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 25, 2019

I think the most important decision is what to use in ModelBase. I'm not sure whether it will be used for serialization, I think that doesn't use property bags in their actual implementation (but will verify).

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 25, 2019

The TypedPropertyBag will start showing a benefit as soon as multiple value typed values are used on a ModelBase. I think this is true for at least 50 % of the cases, so my suggestion is to switch to the TypedPropertyBag by default for the ModelBase.

In case this really bites us, we can release a hotfix to revert this change (keep the PropertyBag the default in ModelBase).

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 25, 2019

Written documentation about both property bags.

@GeertvanHorrik
Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Nov 25, 2019

Will add a virtual member CreatePropertyBag() on ModelBase that can be overridden so users can always hotfix / customize per model if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant