Use ReadOnlySpan instead of Array for argument

[arontsang]

This would reduce the heap allocation per injection, which would help with performance.

[pamidur]

Hi @arontsang!
Do you mean advice argument Attribute[] and use ReadOnlySpan<Attribute> instead to avoid allocations for the collection itself?
If yes, then my thoughts are that I'm not sure it worth it, because: 1. Attribute objects allocations > array allocation , 2. Need to support both Attribute[] and ReadOnlySpan<Attribute> api

The issue with attribute objects - they are not immutable. Otherwise I'd gladly have a prepopulated any readonly collection for them.

What do you think?

[arontsang]

Hi Pamidur,

I mean for

        // Taken from aspect-injector/samples/UniversalWrapper/UniversalWrapper.cs
        [Advice(Kind.Around)]
        public object Handle(
            [Argument(Source.Target)] Func<object[], object> target,
            [Argument(Source.Arguments)] object[] args,
            [Argument(Source.Name)] string name,
            [Argument(Source.ReturnType)] Type retType
            )
        {
            if (typeof(Task).IsAssignableFrom(retType)) //check if method is async, you can also check by statemachine attribute
            {
                var syncResultType = retType.IsConstructedGenericType ? retType.GenericTypeArguments[0] : _voidTaskResult;
                var tgt = target;
                //if (!retType.IsConstructedGenericType)
                //    tgt = new Func<object[], object>(a=>((Task)target(a)).ContinueWith(t=> (object)null));
                return _asyncHandler.MakeGenericMethod(syncResultType).Invoke(this, new object[] { tgt, args, name });
            }
            else
            {
                retType = retType == typeof(void) ? typeof(object) : retType;
                return _syncHandler.MakeGenericMethod(retType).Invoke(this, new object[] { target, args, name });
            }
        }

The line [Argument(Source.Arguments)] object[] args will require the injected code to construct/allocate a object[] array. This of course will need to be collected.

Not sure how much of an impact it adds. But if that was replaced with a ReadOnlySpan<object>, we would be able to stackalloc the object[] instead of heap allocate. This would of course be popped off the stack rather than collected.

On hot path code, this could make a noticeable difference to performance.

There would of course be the following downsides:

• Aspects using the new 'SpanArguments' would have to be synchronous.
• Value types would still have to be boxed, which means they STILL need to be allocated

An alternative to using stackalloc and ReadOnlySpan<object> would be to use ValueTuple. This would of course mean that the user would have to generate multiple Aspect handlers for each possible number of arguments (increasing boilerplate rather than reducing).

[arontsang]

Come to think about it. You can still stackalloc your object[]. But it would make the code unsafe.

[pamidur]

I was thinking about it. What if we preallocate object[] on the heap and then reuse it? This way:

• allocation happens only ones per method
• api still has object[] which values can be modified by aspect (the way it was designed)

[arontsang]

• You would have to allocate a ThreadLocal per method. For frameworks that spawn threads like a Java process, this could leak...

• There would be nothing that prevents 'use after recycle' (unlike with Span<object> where the ref struct prevents that. The user could copy the Span to the heap manually. Additionally, I always though the design was the modify the object[] then pass it to the Func<object[], object>. Which can still be done with ReadOnlySpan.

[pamidur]

agree to both. I do think ReadOnlySpan makes a lot of sense. It could be probably an option for user to decide which API to use. And it will simplify things around references if user already references System.Memory.
This being said I can see the following feature description:

For Source.Arguments advice argument user should be able to choose either use object[] of ReadOnlySpan[]. Weaver produces appropriate code base on the user decision.

In future I also plan to introduce possibility to have generic Around method with signature public T Around<T> where T is target method return type, which could possibly enable user to make async Task advices, thus I see it is required to explicitly forbid advices to be async.

wdyt?

[arontsang]

For your "around<T>", nothing stops the user from trampolining from a synchronous function that returns a Task to an async function. The only "step" they need to take before take off, would be to call ReadOnlySpan<object>.ToArray(), which explicitly allocates stack array (which is unavoidable with Task).

…

On Sun, 11 Oct 2020, 22:23 Oleksandr Hulyi, ***@***.***> wrote: agree to both. I do think ReadOnlySpan makes a lot of sense. It could be probably an option for user to decide which API to use. And it will simplify things around references if user already references System.Memory. This being said I can see the following feature description: For ```Source.Arguments`` advice argument user should be able to choose either use object[] of ReadOnlySpan[]. Weaver produces appropriate code base on the user decision. In future I also plan to introduce possibility to have generic Around method with signature public T Around<T> where T is target method return type, which could possibly enable user to make async Task advices, thus I see it is required to explicitly forbid advices to be async. wdyt? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#136 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA46MLKCIFE6DJPNOHD3BIDSKG5XHANCNFSM4RXJGOGQ> .