Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Hint to "inline" generic type parameter #32815

Closed
artelk opened this issue Feb 25, 2020 · 8 comments
Closed

JIT: Hint to "inline" generic type parameter #32815

artelk opened this issue Feb 25, 2020 · 8 comments
Labels
area-Meta enhancement Product code improvement that does NOT require public API changes/additions

Comments

@artelk
Copy link

artelk commented Feb 25, 2020

Scenario

Let's assume we have a simple buffer writer like this:

SimpleBufferWriter
public sealed class SimpleBufferWriter<T> : IBufferWriter<T>, IDisposable
{
    private readonly IntPtr _memory;
    private readonly int _length;
    private int _index;
    private bool _disposed;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public SimpleBufferWriter(int length)
    {
        this._memory = Marshal.AllocHGlobal(Marshal.SizeOf<T>() * length);
        this._length = length;
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public unsafe Span<T> GetSpan(int sizeHint = 0)
    {
        if (sizeHint == 0)
            sizeHint = 1; // at least 1
        int spanLength = _length - _index;
        if (spanLength < sizeHint)
            ThrowOutOfMemoryException();
        return new Span<T>(Unsafe.Add<T>((void*)_memory, _index), spanLength);
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void Advance(int count)
    {
        var index = _index + count;
        if (index >= _length)
            ThrowInvalidOperationException();
        _index = index;
    }

    public Memory<T> GetMemory(int sizeHint = 0)
    {
        throw new NotImplementedException();
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void Clear()
    {
        _index = 0;
    }

    public unsafe ReadOnlySpan<byte> BytesWritten => new ReadOnlySpan<byte>((void*)_memory, _index);

    public void Dispose()
    {
        if (_disposed) return;
        Marshal.FreeHGlobal(_memory);
        _disposed = true;
    }

    ~SimpleBufferWriter()
    {
        Dispose();
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void ThrowOutOfMemoryException()
    {
        throw new OutOfMemoryException();
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void ThrowInvalidOperationException()
    {
        throw new InvalidOperationException();
    }
}

We created some buffer writer consumer as a generic class parametrized by the buffer writer class like this:

public sealed class HexWriter<TBufferWriter>
    where TBufferWriter : IBufferWriter<byte>
{
    private readonly TBufferWriter _bufferWriter;
    public HexWriter(TBufferWriter bufferWriter) => _bufferWriter = bufferWriter;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void Write(ushort value)
    {
        var span = _bufferWriter.GetSpan(5);
        for (int i = 3; i >= 0; --i)
        {
            var fourBits = (byte)(value & 15);
            span[i] = fourBits < 10 ? (byte)(fourBits + 48) : (byte)(fourBits + 55);
            value >>= 4;
        }
        span[4] = 10; // \n
        _bufferWriter.Advance(5);
    }
}

Also there is a struct wrapping the SimpleBufferWriter:

public readonly struct SimpleBufferWriterWrapper<T> : IBufferWriter<T>
{
    public SimpleBufferWriterWrapper(SimpleBufferWriter<T> bufferWriter) => BufferWriter = bufferWriter;

    public SimpleBufferWriter<T> BufferWriter { [MethodImpl(MethodImplOptions.AggressiveInlining)] get; }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void Advance(int count) => BufferWriter.Advance(count);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public Span<T> GetSpan(int sizeHint = 0) => BufferWriter.GetSpan(sizeHint);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public Memory<T> GetMemory(int sizeHint = 0) => BufferWriter.GetMemory(sizeHint);
}
Benchmark test code
public class Tests
{
    private const int N = 512;
    private SimpleBufferWriter<byte> _bufferWriter;
    private HexWriter<SimpleBufferWriter<byte>> _unwrapped;
    private HexWriter<SimpleBufferWriterWrapper<byte>> _wrapped;

    [GlobalSetup]
    public void Setup()
    {
        _bufferWriter = new SimpleBufferWriter<byte>(N * 20);
        _unwrapped = new HexWriter<SimpleBufferWriter<byte>>(_bufferWriter);
        _wrapped = new HexWriter<SimpleBufferWriterWrapper<byte>>(new SimpleBufferWriterWrapper<byte>(_bufferWriter));
    }

    [Benchmark]
    public void Unwrapped()
    {
        _bufferWriter.Clear();
        for (ushort i = 0; i < N; ++i)
            _unwrapped.Write(i);
    }

    [Benchmark]
    public void Wrapped()
    {
        _bufferWriter.Clear();
        for (ushort i = 0; i < N; ++i)
            _wrapped.Write(i);
    }
}

Test results:

Method Mean Error StdDev
Unwrapped 5.992 us 0.0403 us 0.0357 us
Wrapped 3.648 us 0.0462 us 0.0432 us

The wrapped version is significantly faster than the unwrapped one because the JIT compiler compiles the generic type for each struct type parameter separately so that calls the IBufferWriter implementation methods directly (not via interface) and so it also can inline them. This looks strange but we added another level of indirection to let the JIT compiler get rid of all the levels of indirection and get everything inlined 😄

Feature request

That would be very good if the "inlineable generic parameters" behavior would be (at least optionally) available for reference types as well so it wouldn't be required to create wrapper structs to achieve that.
That could be declared either by special attribute (C# language won't be extended):

class MyClass<[typevar: Inline] T> {}

or by a special syntax

class MyClass<T>
  where T: inline, IFoo
{}
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Feb 25, 2020
@AndyAyersMS
Copy link
Member

Seems like the same ask as in #9682. Changing to area-meta.

@AndyAyersMS AndyAyersMS added area-Meta and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Feb 25, 2020
@artelk
Copy link
Author

artelk commented Feb 25, 2020

Seems like the same ask as in #9682. Changing to area-meta.

Almost the same. But with GenericSpecialization the author of the HexWriter would need to provide the full list of the buffer writers in advance. I would like to avoid that because the HexWriter could be defined in some library and the SimpleBufferWriter could be a client class.

And as far as I understood the [MethodImpl(MethodImplOptions.NonSharedGeneric)] is supposed to be declared for methods only. And that works for all generic parameters at once, not to only one.

@artelk
Copy link
Author

artelk commented Feb 25, 2020

To narrow the scope the attribute could be added to the client-side class:

[GenericSpecializationOn(typeof(HexWriter<>), ...)] // Classes to be "inlined" into. With methodof(...) we would also be able to reference the methods.
public sealed class SimpleBufferWriter<T> : IBufferWriter<T>, IDisposable

So it would be complementary to the GenericSpecialization from #9682 (comment).

Btw, I believe that GenericSpecialization should be applied for individual generic parameters:

TFeature IFeatureCollection.Get<[GenericSpecialization(...)] TFeature>()

Edit:
The GenericSpecializationOn should probably keep generic types and the indices to point exact positions of the generic type parameters:

class GenericSpecializationOnAttribute : Attribute
{
    public GenericSpecializationOnAttribute(params object[] typesAndIndices);
}

class ClassWithThreeParams<T0,T1,T2>{}

[GenericSpecializationOn(
typeof(ClassWithThreeParams<,,>), 2, // T2 parameter of ClassWithThreeParams<T0,T1,T2>
...
)]
public ClientClass
{}

@artelk
Copy link
Author

artelk commented Feb 26, 2020

Both HexWriter and SimpleBufferWriter could be from external libraries and they may know nothing about each other. There has to be another option available for the client-side code:

[assembly: GenericSpecialization(typeof(HexWriter<SimpleBufferWriterWrapper<byte>>))]

So there will be three places where you will be able to declare the generic specification hints:

  1. The target class or method
  2. The class that is supposed to be passed as a generic type parameter for the target type
  3. Assembly level attribute (this probably should work for that assembly only)

@artelk
Copy link
Author

artelk commented Feb 26, 2020

There will be strange corner cases if the target or source classes have subclasses or if both classes are defined in the library and already instantiated there.
I created another proposal #32879

@joperezr
Copy link
Member

joperezr commented Mar 3, 2020

Correct me if I'm wrong but I believe this would actually first require language support for it, so instead of logging this here, we should probably first try logging this on the https://github.com/dotnet/roslyn repo, so that it starts the discussion over there, would that be ok?

Copy link
Contributor

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

@dotnet-policy-service dotnet-policy-service bot added backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity labels Sep 2, 2024
Copy link
Contributor

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2024
@dotnet-policy-service dotnet-policy-service bot removed no-recent-activity backlog-cleanup-candidate An inactive issue that has been marked for automated closure. labels Oct 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta enhancement Product code improvement that does NOT require public API changes/additions
Projects
No open projects
Development

No branches or pull requests

4 participants