[API Proposal]: Embrace spans more in System.Reflection.Emit.

[teo-tsirpanis]

Background and motivation

There are APIs in the System.Reflection.Emit namespace that accept binary data only as byte arrays, forcing callers to allocate an array of exact size for each time they call them. I propose to add overloads to these methods that accept ReadOnlySpan<byte>, reducing needless allocations.

API Proposal

namespace System.Reflection.Emit
{
    public partial class AssemblyBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class ConstructorBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class DynamicILInfo
    {
        public int GetTokenFor(ReadOnlySpan<byte> signature);
        public void SetCode(ReadOnlySpan<byte> code, int maxStackSize);
        public void SetExceptions(ReadOnlySpan<byte> exceptions);
        public void SetLocalSignature(ReadOnlySpan<byte> localSignature);
    }

    public partial class EnumBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class EventBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class FieldBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class GenericTypeParameterBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class MethodBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class ModuleBuilder
    {
        public FieldBuilder DefineInitializedData(string name, ReadOnlySpan<byte> data, FieldAttributes attributes);
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class ParameterBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class PropertyBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class TypeBuilder
    {
        public FieldBuilder DefineInitializedData(string name, ReadOnlySpan<byte> data, FieldAttributes attributes);
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }
}

Going further and adding span overloads for all methods that accept arrays of any type is out of scope and would require changes to the broader Reflection APIs.

API Usage

ModuleBuilder modBuilder = ...;
var typeBuilder = modBuilder.DefineType("Foo");

var dataField = modBuilder.DefineInitializedData("data", (ReadOnlySpan<byte>) new byte[] {1, 2, 3, 4}, FieldAttributes.Private | FieldAttributes.InitOnly);

AssemblyBuilder asmBuilder = ...;
ReadOnlySpan<byte> attributeData = ...;
asm.Builder.SetCustomAttribute(myAttributeConstructor, attributeData);

Alternative Designs

There is the option not to add the DynamicILInfo.Set*** span overloads without sacrificing performance; there are already overloads accepting pointers, but callers would have to resort to unsafe code for no reason, and pinning spans is not supported in F#.

Risks

One would argue how commonly used these APIs are. Maybe they are not worth the effort but their implementations would be simple. As for popularity, upcoming APIs like #60948 provide a compelling use case in dynamic code generators for the spanified DefineInitializedData overloads.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

There are APIs in the System.Reflection.Emit namespace that accept binary data only as byte arrays, forcing callers to allocate an array of exact size for each time they call them. I propose to add overloads to these methods that accept ReadOnlySpan<byte>, reducing needless allocations.

API Proposal

namespace System.Reflection.Emit
{
    public partial class AssemblyBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class ConstructorBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class DynamicILInfo
    {
        public int GetTokenFor(ReadOnlySpan<byte> signature);
        public void SetCode(ReadOnlySpan<byte> code, int maxStackSize);
        public void SetExceptions(ReadOnlySpan<byte> exceptions);
        public void SetLocalSignature(ReadOnlySpan<byte> localSignature);
    }

    public partial class EnumBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class EventBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class FieldBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class GenericTypeParameterBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class MethodBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class ModuleBuilder
    {
        public FieldBuilder DefineInitializedData(string name, ReadOnlySpan<byte> data, FieldAttributes attributes);
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class ParameterBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class PropertyBuilder
    {
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }

    public partial class TypeBuilder
    {
        public FieldBuilder DefineInitializedData(string name, ReadOnlySpan<byte> data, FieldAttributes attributes);
        public void SetCustomAttribute(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
    }
}

Going further and adding span overloads for all methods that accept arrays of any type is out of scope and would require changes to the broader Reflection APIs.

API Usage

ModuleBuilder modBuilder = ...;
var typeBuilder = modBuilder.DefineType("Foo");

var dataField = modBuilder.DefineInitializedData("data", (ReadOnlySpan<byte>) new byte[] {1, 2, 3, 4}, FieldAttributes.Private | FieldAttributes.InitOnly);

AssemblyBuilder asmBuilder = ...;
ReadOnlySpan<byte> attributeData = ...;
asm.Builder.SetCustomAttribute(myAttributeConstructor, attributeData);

Alternative Designs

There is the option not to add the DynamicILInfo.Set*** span overloads without sacrificing performance; there are already overloads accepting pointers, but callers would have to resort to unsafe code for no reason, and pinning spans is not supported in F#.

Risks

One would argue how commonly used these APIs are. Maybe they are not worth the effort but their implementations would be simple. As for popularity, upcoming APIs like #60948 provide a compelling use case in dynamic code generators for the spanified DefineInitializedData overloads.

Author:	teo-tsirpanis
Assignees:	-
Labels:	api-suggestion, area-System.Reflection.Emit, untriaged
Milestone:	-

[steveharter]

Thanks for creating this issue; we are continuing to spanify items as necessary.

Typically, the raw performance of the Emit APIs has not been a concern -- can you explain your priority for this and how its used?

Also the underlying implementation of these APIs needs to be changed of course; currently they perform a defensive copy of the passed-in arrays, creating another array. I think the new APIs should not do this, and document that the underlying array (or whatever backs the span) should not change (although that is somewhat obvious by the ReadOnlySpan).

[teo-tsirpanis]

My priority is not high, but I would make use of DefineInitializedData if approved.

currently [the APIs] perform a defensive copy of the passed-in arrays, creating another array.

I might be wrong, but my impression is that this happens only for the DynamicILInfo.Set*** family of methods. The others seem to call a QCall that marshals the array into a pointer and passes it to the runtime, without any apparent defensive copies (in managed memory at least, the native part of the runtime would of course make a copy).

[steveharter]

You're right, the defensive copy doesn't apply to all of the methods. My main point here is that we can further optimize these with the PR.

I'll keep this feature open and assign to 7.0 for now. It may make sense to do this at the same time as AssemblyBuilder.Save refactoring. The idea is that these emit calls would instead forward to MetadataWriter instead of QCalls when it's time to update the metdata\IL.

[steveharter]

Moving to 8.0.

We should embrace Spans more in Reflection including the Emit APIs as originally mentioned. However, another important area that could use Span is the many APIs that return arrays (e.g. MethodInfo.GetParameters()) where a defensive copy is made since arrays cannot be made read-only and because we need to prevent cached array elements from being replaced in the array. For these, it is possible to use ReadOnlySpan there instead which will improve performance by avoiding the defensive copy.