Universal Wrapper

[pamidur]

look good so far. Do you think it is ok to mute the exceptions in our case?

[StefH]

look good so far. Do you think it is ok to mute the exceptions in our case?

Maybe make this configurable?

[StefH]

@pamidur : I did move + fix some code and added a working example to the "Logging" console app.

[StefH]

I love your PR! It can make the use of Aspect Injector even more convenient. I have added a few questions inline to discuss.

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?

[pamidur]

I love your PR! It can make the use of Aspect Injector even more convenient. I have added a few questions inline to discuss.

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. Maybe 'AspectInjector.Framework' in AspectInjector. sln?

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

[pamidur]

One more thing to discuss. All attributes instances are created in place of calling aspect now. Which means we instantiate attributes as many times as we call a target method. This is somewhat related to #137

The benefits of current approach: no side effects, data store in attribute is disposed the moment target method ends The downside: performance

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.

[StefH]

One more thing to discuss. All attributes instances are created in place of calling aspect now. Which means we instantiate attributes as many times as we call a target method. This is somewhat related to #137

The benefits of current approach: no side effects, data store in attribute is disposed the moment target method ends The downside: performance

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 ? Like : could this issue https://github.com/pamidur/aspect-injector/issues/147 also help on that ?

[pamidur]

One more thing to discuss. All attributes instances are created in place of calling aspect now. Which means we instantiate attributes as many times as we call a target method. This is somewhat related to #137 The benefits of current approach: no side effects, data store in attribute is disposed the moment target method ends The downside: performance 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 ? Like : could this issue #147 also help on that ?

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

[sergeprozorov]

Hi guys, This is a PR with a feature I am really looking forward to :)

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;
        }
    }
}

[pamidur]

@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

[sergeprozorov]

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? Regarding delegate .Invoke performance, it is quite close to direct calls. There are many benchmarks out there showing they are 10-20% slower, than direct calls. Whereas MethodInfo.Invoke is 600-1000 times slower than those.

And I am happy to hear from you too! Your framework is the real thing, very good work, mate, thanks!

[StefH]

@pamidur and @sergeprozorov Dp you have any update on this PR?

[pamidur]

@pamidur and @sergeprozorov Dp you have any update on this PR?

Hi, I'm ready to review and merge it. Just tell me when all the work on this one is done.

[StefH]

@pamidur I think I have all the code ready, just review and comment if you want things changed. (Or change it in my branch yourself)

[pamidur]

Hi @StefH , I have made quite a few changes:

• made chaining two and more universal aspects more accurate
• added caching
• moved some logic to attributes

So I'd say it is pretty close now, we could merge and release it as a preview. Till full release will need:

• tests to cover all scenarios: aspects order, asyncs, exceptions etc
• readme.md desribing what this aspect is for and how to use it (see example https://github.com/pamidur/aspect-injector/blob/master/samples/src/Freezable/readme.md)

wdyt?

[StefH]

@pamidur Looks good.

I did add 3 small comments, however I'm not sure you can see these because I'm the author?

[StefH]

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.

[pamidur]

I have it merged @StefH , please continue your testing and then we'll work on releasing it

[StefH]

@pamidur Can you please create a new preview from aspect-injector so that I can reference that one for the universal ?

[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.

[StefH]

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.

[pamidur]

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

[StefH]

I did use this code, and for my project it still works fine.

[pamidur]

Sounds like we can make a release then!

[StefH]

@pamidur Did you have time yet to release this?

[pamidur]

Published here https://www.nuget.org/packages/Aspects.Universal/