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

Microsoft.Toolkit.HighPerformance package (part 2) #3273

Merged

Conversation

@Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented May 7, 2020

Follow up to #3128

PR Type

What kind of change does this PR introduce?

  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes, no api changes)

What is the new behavior?

This PR includes the following changes to the Microsoft.Toolkit.HighPerformance package:

  • Added new .NET Standard 1.4, .NET Core 2.1 and .NET Core 3.1 targets
  • Refactored the runtime-dependent APIs to ensure compatibility outside of .NET Core. In particular, the .NET Standard targets no longer make any assumptions on the actual runtime now, which make all the available APIs valid (and much safer) on Mono/Unity as well.
  • Added new APIs (see below)
  • Minor code refactoring and style tweaks

Below is a breakthrough of all the new types and APIs added in this PR:

Microsoft.Toolkit.HighPerformance.Buffers
namespace Microsoft.Toolkit.HighPerformance.Buffers
{
    public interface IBuffer<T> : IBufferWriter<T>
    {
        ReadOnlyMemory<T> WrittenMemory { get; }
        ReadOnlySpan<T> WrittenSpan { get; }
        int WrittenCount { get; }
        int Capacity { get; }
        int FreeCapacity { get; }
        void Clear();
    }

    public sealed class ArrayPoolBufferWriter<T> : IBuffer<T>, IMemoryOwner<T>
    {
        // Same as before, but now implementing IBuffer<T>
    }

    public sealed class MemoryBufferWriter<T> : IBuffer<T>
    {
        public MemoryBufferWriter(Memory<T> memory);
        public ReadOnlyMemory<T> WrittenMemory { get; }
        public ReadOnlySpan<T> WrittenSpan { get; }
        public int WrittenCount { get; }
        public int Capacity { get; }
        public int FreeCapacity { get; }
        public void Clear();
        public void Advance(int count);
        public Memory<T> GetMemory(int sizeHint = 0);
        public Span<T> GetSpan(int sizeHint = 0);
        public override string ToString();
    }
}
Microsoft.Toolkit.HighPerformance.Extensions
namespace Microsoft.Toolkit.HighPerformance.Extensions
{
    public static class BoolExtensions
    {
        public static int ToBitwiseMask32(this bool flag);
        public static long ToBitwiseMask64(this bool flag);
    }

    public static class IBufferWriterExtensions
    {
        public static void Write<T>(this IBufferWriter<byte> writer, T value)
            where T : unmanaged;
        public static void Write<T>(this IBufferWriter<T> writer, T value);        
        public static void Write<T>(this IBufferWriter<byte> writer, ReadOnlySpan<T> span)
            where T : unmanaged;
#if NETSTANDARD2_0
        public static void Write<T>(this IBufferWriter<T> writer, ReadOnlySpan<T> span);
#endif
    }

    public static class ObjectExtensions
    {
        public static IntPtr DangerousGetObjectDataByteOffset<T>(this object obj, ref T data);
        public static ref T DangerousGetObjectDataReferenceAt<T>(this object obj, IntPtr offset);
    }

    public static class ReadOnlySpanExtensions
    {
        public static ref readonly T DangerousGetLookupReferenceAt<T>(this ReadOnlySpan<T> span, int i);
    }

    public static class StreamExtensions
    {
#if NETSTANDARD2_0
        public static ValueTask<int> ReadAsync(this Stream stream, Memory<byte> buffer, CancellationToken cancellationToken = default);
        public static ValueTask WriteAsync(this Stream stream, ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default);
        public static int Read(this Stream stream, Span<byte> buffer);
        public static void Write(this Stream stream, ReadOnlySpan<byte> buffer);
#endif
        public static T Read<T>(this Stream stream)
            where T : unmanaged;
        public static void Write<T>(this Stream stream, in T value)
            where T : unmanaged;
    }
}
Microsoft.Toolkit.HighPerformance.Helpers
namespace Microsoft.Toolkit.HighPerformance.Helpers
{
    public static class BitHelper
    {
        public static bool HasLookupFlag(uint table, int x, int min = 0);
        public static uint ExtractRange(uint value, byte start, byte length);
        public static void SetRange(ref uint value, byte start, byte length, uint flags);
        public static uint SetRange(uint value, byte start, byte length, uint flags);
        public static bool HasLookupFlag(ulong table, int x, int min = 0);
        public static ulong ExtractRange(ulong value, byte start, byte length);
        public static void SetRange(ref ulong value, byte start, byte length, ulong flags);
        public static ulong SetRange(ulong value, byte start, byte length, ulong flags);
    }
}

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Marking as draft PR for now, so far I've added the follow up APIs and did all the various refactorings and bug fixes, but I still need to go through @michael-hawker's requested changes in #3128. Will remove the draft PR status when that part is done as well 🚀

Done, ready for review! 🎉

Sergio0694 added 30 commits Mar 15, 2020
Changes from official master
Changes from master
Changes from master
Copy link
Member

@michael-hawker michael-hawker left a comment

I'm good with this if @azchohfi and/or @tannergooding are

@Sergio0694
Copy link
Member Author

@Sergio0694 Sergio0694 commented May 11, 2020

@tannergooding this is the PR I mentioned the other day 😄
This one in particular is mostly refactorings, proper support for the various frameworks, and some new APIs. The vectorized code I mentioned the other day, if you want to have a look, is here:

Other things you might be interested in are the BitHelper class, which uses some intrinsics on .NET Core 3.1, and the various buffers/streams. Any feedbacks are welcome 😊

@tannergooding
Copy link

@tannergooding tannergooding commented May 11, 2020

Taking a look

@tannergooding
Copy link

@tannergooding tannergooding commented May 11, 2020

Skimmed most of it with a couple comments and reviewed the SIMD code specifically. Overall the logic looks good to me.

@Sergio0694
Copy link
Member Author

@Sergio0694 Sergio0694 commented May 11, 2020

Hey @tannergooding - thank you so much for taking the time to go through the PR, I really appreciate it! Glad to hear the overall logic seemed ok, I'll go through you various comments now 😄

@michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented May 12, 2020

@tannergooding thanks for taking a look for us!

@Sergio0694 let me know when you've resolved all the discussions and we're good to merge this.

@Sergio0694 Sergio0694 mentioned this pull request May 12, 2020
7 of 7 tasks complete
@Sergio0694
Copy link
Member Author

@Sergio0694 Sergio0694 commented May 12, 2020

@michael-hawker All good, I've added those extra remarks to ToBitwise32 as suggested:

/// <remarks>
/// This method does not contain branching instructions, and it is only guaranteed to work with
/// <see cref="bool"/> values being either 0 or 1. Operations producing a <see cref="bool"/> result,
/// such as numerical comparisons, always result in a valid value. If the <see cref="bool"/> value is
/// produced by fields with a custom <see cref="System.Runtime.InteropServices.FieldOffsetAttribute"/>,
/// or by using <see cref="Unsafe.As{T}"/> or other unsafe APIs to directly manipulate the underlying
/// data though, it is responsibility of the caller to ensure the validity of the provided value.
/// </remarks>
[Pure]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int ToBitwiseMask32(this bool flag)

Also made those refactorings that were mentioned (eg. integrating an internal version of the .NET Standard 2.1 attributes, which is a change I've done to #3167 as well for consistency). The part about further possible optimizations (from here) can be addressed in a follow up PR if we decide the complexity is worth the speedup, as that change would be a bit tricky to implement.

As far as I can tell, the PR is ready to go now! 😄

Sergio0694 added 2 commits May 12, 2020
@michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented May 13, 2020

@Sergio0694 let's get this in for now since things looked good to @tannergooding. And if we see some good usage of this new library we can investigate in the future spending more time for the complex optimization. Sound good?

@michael-hawker michael-hawker merged commit 059cf83 into windows-toolkit:master May 13, 2020
3 checks passed
3 checks passed
Toolkit-CI Build #6.1.0-build.166+81e6fd3c2b succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
@michael-hawker michael-hawker added this to the 6.1 milestone May 13, 2020
@Sergio0694
Copy link
Member Author

@Sergio0694 Sergio0694 commented May 13, 2020

@michael-hawker Sounds perfect, absolutely!

Also, mission complete! 🎉🎉🎉

This has been quite the journey, thank you, @azchohfi and @tannergooding so much! 😊

@Sergio0694 Sergio0694 mentioned this pull request Jun 11, 2020
4 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.