Cache boxed value type values in ModelBase

[GeertvanHorrik]

Description of Change

Cache boxed values.

Issues Resolved

• fixes Implement caching of boxed values in ModelBase to decrease memory footprint #1437

API Changes

• ModelBase::SetValue
• ModelBase::SetValueFastButInsecure
• ModelBase::GetValueFastButInsecure

Platforms Affected

• All
• WPF
• UWP
• iOS
• Android

Behavioral Changes

None

Testing Procedure

PR Checklist

• I have included examples or tests
• I have updated the change log
• I am listed in the CONTRIBUTORS file
• Rebased on top of the target branch at time of PR
• Changes adhere to coding standard

[ghost]

Just more of a curiosity. I see you removed the type constraint but would have it made more sense to constrain it against IConvertible? It's supported on all the platforms Catel is targeting and makes more sense than struct for what this is being used for.

[GeertvanHorrik]

Cool, was not aware of this interface. Will investigate, I want to enforce value types.

[ghost]

Yeah it's perfect for when you want to enforce only primitive types (or anything that implements IConvertible) in an generic since struct can introduce a whole can of worms (also avoids having to do ValueType or IsPrimitive checks elsewhere).

[GeertvanHorrik]

It does support reference values as well (e.g. a string), and that's something we definitely don't want to cache). But we can enforce that simply by checking for value type first.

[GeertvanHorrik]

And now I remember why I didn't enforce it to anything. We use this in this method:

        protected static object GetObjectValue<TValue>(TValue value)
        {
            object objectValue = null;

            if (typeof(TValue).IsValueTypeEx())
            {
                objectValue = BoxingCache<TValue>.Default.GetBoxedValue(value);
            }
            else
            {
                objectValue = value;
            }

            return objectValue;
        }

Then we would have to add the constraint there as well, but we want to support reference types there as well (and at runtime check which path to take).

[GeertvanHorrik]

Hmmm, maybe I can implement it here:

Use typeof(IConvertible).IsAssignableFrom(type) for UAP. Will need to profile performance first though.

[ghost]

Maybe this may be better then?

public static bool IsValueTypeEx(this Type type)
{
    return (type is IConvertible);
}

Since all primitive types derive from this interface that should be all you'd need. There is the obvious side effect that if someone actually goes through the trouble of implementing IConvertible it ends up returning true but I've never seen anyone actually do that.

[GeertvanHorrik]

Hmmm, IsAssignableFrom is an extension method, I wonder whether it does this under the hood anyway:

        public static bool IsAssignableFromEx(this Type type, Type typeToCheck)
        {
            Argument.IsNotNull("type", type);
            Argument.IsNotNull("typeToCheck", typeToCheck);

#if UAP_DEFAULT
            return type.IsAssignableFrom(typeToCheck);
            //return type.GetTypeInfo().IsAssignableFrom(typeToCheck.GetTypeInfo());
#else
            return type.IsAssignableFrom(typeToCheck);
#endif
        }

[GeertvanHorrik]

Maybe this may be better then?

public static bool IsValueTypeEx(this Type type)
{
    return (type is IConvertible);
}

Since all primitive types derive from this interface that should be all you'd need. There is the obvious side effect that if someone actually goes through the trouble of implementing IConvertible it ends up returning true but I've never seen anyone actually do that.

This definitely makes sense for UAP, but I think it's still faster to read a property value than to check for type (but not sure, and we are talking nano-optimizations here).

[ghost]

Agreed, if there’s serious concerns about performance here (doubtful) dotTrace can be used later.

[ghost]

This is a good example of what would benefit from the expression tree setter and would speed up serialization.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.