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

Consider using expression trees in ObjectAdapter #1439

Closed
GeertvanHorrik opened this issue Oct 13, 2019 · 30 comments · Fixed by #1447
Closed

Consider using expression trees in ObjectAdapter #1439

GeertvanHorrik opened this issue Oct 13, 2019 · 30 comments · Fixed by #1447
Assignees
Labels
Milestone

Comments

@GeertvanHorrik
Copy link
Member

@GeertvanHorrik GeertvanHorrik commented Oct 13, 2019

Suggested in #1438 by @vaecors :

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

and

The FastExpressionCompiler is an optional behavioral change and is not necessarily required for those methods. We used it to speed up cold starts and the fact the library it sits in already depends on it due to our use of Mapster. I made about one comment concerning one of the changes. I also was a bit curious of the use of IsValueType vs IsPrimitive type for one of the calling methods but that's more of a preference I feel more than an advantage of A vs B.

@GeertvanHorrik GeertvanHorrik changed the title Consider using Consider using FastExpressionCompiler in ObjectAdapter Oct 13, 2019
@GeertvanHorrik GeertvanHorrik self-assigned this Oct 13, 2019
@GeertvanHorrik GeertvanHorrik added this to the 5.12.0 milestone Oct 13, 2019
@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 13, 2019

Just as a note, if you want to remove the FastExpressionCompiler dependency from the example class, simply replace any CompileFast calls with Compile.

@GeertvanHorrik GeertvanHorrik changed the title Consider using FastExpressionCompiler in ObjectAdapter Consider using expression trees in ObjectAdapter Oct 16, 2019
@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 16, 2019

@vaecors I wanted to start working on this this evening. I'll start with expression trees, and then we might consider adding FastExpressionCompiler.

Do you have a good example of expression trees when you really don't know anything about the type? How do you ensure performance (e.g. via caching, or ... )?

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 16, 2019

And is the .Compile available on all platforms or will it be limited on some (e.g. Xamarin, UWP, etc)?

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 16, 2019

@GeertvanHorrik Yes .Compile is available on all target platforms. I'm not sure about FastExpressionCompiler though as we only target .NET Core and WPF. As for handling unknown types if you look at the gist I provided, it has methods to create "untyped" setters (i.e. Action<T, object> and Func<T, object>).

As we cache the Action/Func within a dictionary (where the property name is the key), we haven't noticed a real performance issue. In a nutshell we have a custom ExcelReader class that uses the OpenXML SDK and using a custom IDataReader class we also created, projects all the objects into concrete types, it projected a 1 million row workbook in about 2-3 seconds.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 16, 2019

I just took a quick peak at FastExpressionCompiler, it targets .NET Standard which I believe can be imported into Xamarin/UWP? We don't really do development on those targets so I can't give you a concrete conclusion.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 16, 2019

Thanks for the input! Will finalize the xml serializer, then this is the next thing I will look into. Will run the benchmarks tonight to see if we made any real progress :)

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 16, 2019

Anytime, I'm actually toying with the idea of doing an implementation where the typed getter/setters could be used against a class in a straight forward fashion (we primarily use only the untyped variant not really worrying too much about inline optimizations).

The premise being the implementation (most likely an instantiated class) would be wrapped around the target class and internal dictionary caches for all primitive types would be maintained (properties shredded out via their primitive type) to avoid any IL boxing/unboxing that could occur. Public methods would then be exposed to handle this. I won't have time though for about the next 3 weeks to really explore that though.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 16, 2019

Sounds interesting. Let's first create some helper methods / wrappers to use expressions instead of reflection. After that, we can actually wrap it into easy to use classes. Sounds like a great idea!

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 16, 2019

Yeah I was thinking about it quite a bit today since the properties of a target class already have to be enumerated through to build the expressions it's not out of the realm to group those by their type and direct them to the appropriate cached collection and take advantage of strongly typed getter/setter expression tree. It may be wise to also throw an InvalidOperationException for anything that is a non-primitive or DBNull because of complexity issues but I don't know if that's the best course of action.

I went ahead and made a blank repository for now, so when I get free bits of time I'll commit any ideas towards it in-case you want to keep an eye on it.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 16, 2019

@GeertvanHorrik There was a signature goof in the gist for the ExpressionBuilder. I have updated it.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 16, 2019

@GeertvanHorrik Actually there are some additional issues with the typed getter/setters. I need to re-write these methods so don't use the code for now (I should have it updated tonight though).

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 16, 2019

@GeertvanHorrik I fixed the associated issues with the strongly typed setters/getters. I also re-wrote them to use expressions instead of delegates so they can take advantage of CompileFast.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 16, 2019

@GeertvanHorrik It actually didn't take as long as I thought (about 2 hours), there's a full set of code with benchmarks on the FastPropertyInvokerTest repository, have fun.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 17, 2019

So approx. 2 x faster. Any idea about the cost off the initial cold start?

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 17, 2019

will run all the benchmarks tonight without the expression thingie

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 17, 2019

Any idea about the cost off the initial cold start?

I honestly never timed it but correlating with the benchmarks of FastExpressionCompiler and the fact it's only doing public properties (private and protected are ignored) I would imagine it's negligible for most classes. If you start having classes with 50 or more properties the linear operation .GetProperties() might hurt but it's probably getting into premature optimization territory at this point.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 17, 2019

I will update this in ObjectAdapter.

If ModelBase => use directly
else use these expression trees

The only downside is that we can't use generic (we really don't know anything), but we can build up this using reflection (1 time slow). So as soon as objects are being re-used, we will see benefit.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 17, 2019

It's my understanding that neither of those classes had Generic support anyway or am I missing some context here?

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 17, 2019

This seems generic:

https://github.com/vaecors/FastPropertyInvokerTest/blob/master/FastPropertyInvokerTest/FastPropertyInvoker.caches.g.cs#L15

But I had something to drink, maybe I should check tomorrow ;-)

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 17, 2019

Right now I'm sync'd up with you, yeah the FastPropertyInvoker is a generic to facilitate the building of the caches and functions against the target class. I now see what you mean by doing the reflection as you need to grab the derived type then instantiate it. May even be able to avoid reflection there and use an expression tree to instantiate it (we do this with classes in our system).

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 19, 2019

Introducing a new interface Catel.Data.IObjectAdapter. This will be used inside the Catel.Runtime.Serialization.ObjectAdapter internally (but that does more, such as creating the member values required for serialization).

The names are a bit confusing, but for any object adapters, the Catel.Data one should be used:

/// <summary>
/// Object adapter allowing to customize reflection and member mappings.
/// </summary>
public interface IObjectAdapter
{
    /// <summary>
    /// Gets the member value of the instance.
    /// </summary>
    /// <typeparam name="TValue">The type of the value to retrieve.</typeparam>
    /// <param name="instance">The instance.</param>
    /// <param name="memberName">The member name.</param>
    /// <returns>The member value.</returns>
    TValue GetMemberValue<TValue>(object instance, string memberName);

    /// <summary>
    /// Sets the member value of the instance.
    /// </summary>
    /// <param name="instance">The instance.</param>
    /// <param name="memberName">The member name.</param>
    /// <param name="value">The value.</param>
    /// <returns>The member value.</returns>
    void SetMemberValue<TValue>(object instance, string memberName, TValue value);
}

Catel will have 2 implementations:

  • ReflectionObjectAdapter (only uses reflection)
  • ExpressionTreeObjectAdapter (uses expression trees)

We'll make the generation of the expression tree virtual protected. This allows anyone to use CompileFast without Catel.Core having to reference external dependencies. I think this is the best solution I can come up with.

Will update the WIP PR soon.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 19, 2019

@GeertvanHorrik you may want to look at how Mapster handles configuring FastExpressionCompiler https://github.com/MapsterMapper/Mapster/wiki/FastExpressionCompiler

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 19, 2019

Updated interface definition:

/// <summary>
/// Object adapter allowing to customize reflection and member mappings.
/// </summary>
public interface IObjectAdapter
{
    /// <summary>
    /// Gets the member value of the instance.
    /// </summary>
    /// <typeparam name="TValue">The type of the value to retrieve.</typeparam>
    /// <param name="instance">The instance.</param>
    /// <param name="memberName">The member name.</param>
    /// <param name="value">The member value to update.</param>
    /// <returns><c>true</c> if the member was retrieved; otherwise <c>false</c>.</returns>
    bool GetMemberValue<TValue>(object instance, string memberName, ref TValue value);

    /// <summary>
    /// Sets the member value of the instance.
    /// </summary>
    /// <param name="instance">The instance.</param>
    /// <param name="memberName">The member name.</param>
    /// <param name="value">The value.</param>
    /// <returns><c>true</c> if the member was set successfully; otherwise <c>false</c>.</returns>
    bool SetMemberValue<TValue>(object instance, string memberName, TValue value);
}
GeertvanHorrik added a commit that referenced this issue Oct 20, 2019
@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 20, 2019

Reflection implementation is done. Now we "only" have to implement the expression tree one.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 20, 2019

Just an update: PR #1447 contains a FastMemberInvoker that is strongly based on the great work by @vaecors . I've added virtual Compile methods so users could actually override this and use CompileFast instead.

In the ExpressionTreeObjectAdapter, I'll implement a virtual method to create the FastMemberInvoker class so it can be overridden with a custom implementation (that uses FastCompile). I think this is going to work great. Next step is also adding fields support.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 20, 2019

See https://github.com/Catel/Catel/pull/1447/files#diff-63eb25c26c7957d68adb286f4ad4a096R96 on where to override the construction of the FastMemberInvoker inside the ExpressionTreeObjectAdapter.

See https://github.com/Catel/Catel/pull/1447/files#diff-63eb25c26c7957d68adb286f4ad4a096R96 to see how to customize the compile methods.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 21, 2019

@vaecors just pushed a new large update. I've implemented the FastMemberInvoker, both fields and properties are supported. Also implemented the ExpressionTreeObjectAdapter (registered by default).

The FastMemberInvoker does lazy-compilation (instead of premature compilation) so only member access expressions that are required will be compiled.

The only thing I am not happy about (yet) is the part where TValue needs to be casted (but needs to be boxed first):

https://github.com/Catel/Catel/pull/1447/files#diff-f3c1ab2eb24ea2e765b7851bb32201a5R43

I think the rest is starting to look nice.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Oct 21, 2019

@GeertvanHorrik Looking good, I have confidence this will greatly help performance in the 5.12 release. Looking at your note about boxing to object (only to then unbox it), I wonder if it pays to look into building a series of delegates to handle primitive type conversions similar to this but maybe a bit more cleaner/streamlined. Obviously it would only work for primitive types but a T4 template could be shimmed to generate most of the code I think.

Edit: We do something less drastic via extension methods with converting strings to various types via the TryParse and Parse methods like "1".ToInt() and so forth.

@GeertvanHorrik

This comment has been minimized.

Copy link
Member Author

@GeertvanHorrik GeertvanHorrik commented Oct 22, 2019

Created a separate ticket. Will merge this PR so we can actually start testing this with real apps.

@lock

This comment has been minimized.

Copy link

@lock lock bot commented Oct 26, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
1 participant
You can’t perform that action at this time.