Open arontsang opened 3 years ago
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?
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:
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).
Come to think about it. You can still stackalloc your object[]
. But it would make the code unsafe
.
I was thinking about it. What if we preallocate object[] on the heap and then reuse it? This way:
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
.
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?
For your "around
On Sun, 11 Oct 2020, 22:23 Oleksandr Hulyi, notifications@github.com 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
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 https://github.com/pamidur/aspect-injector/issues/136#issuecomment-706712200, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA46MLKCIFE6DJPNOHD3BIDSKG5XHANCNFSM4RXJGOGQ .
This would reduce the heap allocation per injection, which would help with performance.