Universal Wrapper #148
Universal Wrapper #148
Conversation
|
look good so far. |
Maybe make this configurable? |
|
@pamidur : I did move + fix some code and added a working example to the "Logging" console app. |
Thanks. Please note that I've only created some Aspects and some attributes. This is just a start, more specific ones can still be added. Is the name "AspectInjector.Universal" good and clear enough? |
I was thinking about it too. One option was 'Aspects.Framework' other was to make it part of AspectInjector package. For now let's keep it as is tho, I want to see how it goes. I feel like to make this framework really shine it might require some significant changes to the core and thus require bumping to 3.x |
|
One more thing to discuss. The benefits of current approach: no side effects, data store in attribute is disposed the moment target method ends possible solution - cache attribute instances, but it is tricky (because generics) and we doing so we might as well cache all other things like name, type, and metadata. |
Could this be solved at AspectInjector library level ? |
Yes, It can only be fixed on AspectInjector library level , I`m trying to evaluate if it should be fixed at all. #147 is definitely part of it |
|
Hi guys, The only note I would like to add, is it possible to re-work the wrapper in a way not to use MethodInfo.Invoke? It is slow as hell, comparing to a static or a virtual call. A good option would be using delegates, they are almost as fast as virtual ones. I have this piece of code below, where I use delegates. Could you have a look, please? internal static class UniversalWrapper
{
private delegate object? DynamicWrapperDelegate(Func<object[], object> target, object[] args);
private static readonly Dictionary<Type, DynamicWrapperDelegate> _delegateCache = new Dictionary<Type, DynamicWrapperDelegate>();
private static readonly MethodInfo _asyncGenericHandler =
typeof(UniversalWrapper).GetMethod(nameof(WrapAsync), BindingFlags.NonPublic | BindingFlags.Static);
private static readonly Type _voidTaskResult = Type.GetType("System.Threading.Tasks.VoidTaskResult");
public static object? Wrap(Func<object[], object> target, object[] args, Type returnType)
{
if (typeof(Task) == returnType)
{
return WrapVoidAsync(target, args);
}
if (!typeof(Task).IsAssignableFrom(returnType))
{
// It is a sync call, no need to wrap anything around, just return the call result.
return target(args);
}
return GetFromCache(returnType).Invoke(target, args);
}
private static async Task<T> WrapAsync<T>(Func<object[], object> target, object[] args)
{
return await ((Task<T>)target(args)).ConfigureAwait(false);
}
private static async Task WrapVoidAsync(Func<object[], object> target, object[] args)
{
await ((Task)target(args)).ConfigureAwait(false);
}
private static DynamicWrapperDelegate GetFromCache(Type returnType)
{
if (_delegateCache.ContainsKey(returnType))
{
return _delegateCache[returnType];
}
lock (_delegateCache)
{
if (_delegateCache.ContainsKey(returnType))
{
return _delegateCache[returnType];
}
var syncResultType = returnType.IsConstructedGenericType ? returnType.GenericTypeArguments[0] : _voidTaskResult;
MethodInfo method = _asyncGenericHandler.MakeGenericMethod(syncResultType);
var @delegate = (DynamicWrapperDelegate)Delegate.CreateDelegate(typeof(DynamicWrapperDelegate), method);
_delegateCache.Add(returnType, @delegate);
return @delegate;
}
}
} |
|
@sergeprozorov, I was actually thinking about something like this, but I though about constructing Expressions and compile them instead of dynamically creating delegates. Idk which one will be faster, On one hand compiling expression takes time, on the other hand delegate.Invoke isn't particularly fast either. It needs some testing. Feel free to contribute! oftopic: I glad to hear from you, it's been a long time since Exigen Services |
|
Regarding Expression.Compile, it essentially produces a delegate, so it is a question of convenience and habits. But bear in mind, sometimes it doesn't work as fast as you might expect, Why is Func<> created from Expression<Func<>> slower than Func<> declared directly? And I am happy to hear from you too! Your framework is the real thing, very good work, mate, thanks! |
|
@pamidur and @sergeprozorov |
Hi, I'm ready to review and merge it. Just tell me when all the work on this one is done. |
|
@pamidur I think I have all the code ready, just review and comment if you want things changed. |
…improved a bit on attributes structure
|
Hi @StefH , I have made quite a few changes:
So I'd say it is pretty close now, we could merge and release it as a preview. Till full release will need:
wdyt? |
|
@pamidur I did add 3 small comments, however I'm not sure you can see these because I'm the author? |
|
Looks good to me, I'll take this code and try it in my existing project. If that works fine, I think a preview version can be released on NuGet. |
|
I have it merged @StefH , please continue your testing and then we'll work on releasing it |
|
@pamidur |
|
@StefH , you mean preview for aspect-injector or universal wrapper? If you mean aspect-injector there were little change since 2.6.0-preview so we can use it. |
|
In this case "aspect-injector", to be sure all new code is there. For "universal wrapper" : I'll just compile it in my project. No need for NuGet yet. |
|
It doesn't look like there were changes to aspect-injector itself since https://github.com/pamidur/aspect-injector/releases/tag/2.6.0-pre1 which I've merged into your branch before, so it should be fully compatible |
|
I did use this code, and for my project it still works fine. |
|
Sounds like we can make a release then! |
|
@pamidur |
|
Published here https://www.nuget.org/packages/Aspects.Universal/ |
No description provided.