Add System.Memory reference #31162

agocke · 2018-11-14T00:22:06Z

Allows us to replace some unsafe code with Span. I anticipate more complex
uses in the future.

Here are the benchmarks.

Before

BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.345 (1803/April2018Update/Redstone4)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=2.2.200-preview-009431
  [Host] : .NET Core 2.1.6 (CoreCLR 4.6.27019.06, CoreFX 4.6.27019.05), 64bit RyuJIT
  Core   : .NET Core 2.1.6 (CoreCLR 4.6.27019.06, CoreFX 4.6.27019.05), 64bit RyuJIT

Job=Core  Runtime=Core  Server=True

                Method |      Mean |     Error |    StdDev |    Median |     Gen 0 | Allocated |
---------------------- |----------:|----------:|----------:|----------:|----------:|----------:|
               Parsing |  96.86 ms |  2.568 ms |  7.571 ms |  96.89 ms |         - |   4.25 MB |
 CompileMethodsAndEmit | 688.64 ms | 13.659 ms | 38.971 ms | 681.50 ms | 1000.0000 |  43.97 MB |
     SerializeMetadata | 231.56 ms | 10.722 ms | 31.613 ms | 244.91 ms |         - |  18.06 MB |

After

BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.345 (1803/April2018Update/Redstone4)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=2.2.200-preview-009431
  [Host] : .NET Core 2.1.6 (CoreCLR 4.6.27019.06, CoreFX 4.6.27019.05), 64bit RyuJIT
  Core   : .NET Core 2.1.6 (CoreCLR 4.6.27019.06, CoreFX 4.6.27019.05), 64bit RyuJIT

Job=Core  Runtime=Core  Server=True

                Method |      Mean |     Error |    StdDev |    Median |     Gen 0 | Allocated |
---------------------- |----------:|----------:|----------:|----------:|----------:|----------:|
               Parsing |  95.71 ms |  2.381 ms |  7.021 ms |  95.44 ms |         - |   3.59 MB |
 CompileMethodsAndEmit | 708.76 ms | 19.100 ms | 55.108 ms | 707.51 ms | 1000.0000 |  44.02 MB |
     SerializeMetadata | 199.04 ms |  8.754 ms | 24.834 ms | 191.37 ms |         - |  18.06 MB |

There does not appear to be any statistically significant difference.

CyrusNajmabadi · 2018-11-14T00:26:22Z

src/Compilers/Core/Portable/InternalUtilities/Hash.cs

        /// <param name="isAscii">True if the sequence contains only characters in the ASCII range.</param>
        /// <returns>The FNV-1a hash of <paramref name="data"/></returns>
-        internal static unsafe int GetFNVHashCode(byte* data, int length, out bool isAscii)
+        internal static int GetFNVHashCode(ReadOnlySpan<byte> data, out bool isAscii)


this makes me so happy :)

Also replaces some unsafe code with Span<T>.

Therzok · 2018-11-14T02:02:23Z

src/Compilers/Core/Portable/InternalUtilities/StringTable.cs

-            return true;
-        }
+        internal static bool TextEquals(string array, ReadOnlySpan<char> text)
+            => text.Equals(array.AsSpan(), StringComparison.Ordinal);


~~I think this should be SequenceEquals.~~

Nevermind!

Mirrors dotnet/roslyn#31162

jcouv · 2018-11-14T23:58:03Z

src/Compilers/Core/Portable/InternalUtilities/StringTable.cs

        {
 #if DEBUG
-            for (var i = 0; i < length; i++)
+            for (var i = 0; i < ascii.Length; i++)
            {
                Debug.Assert((ascii[i] & 0x80) == 0, "The byte* input to this method must be valid ASCII.");


byte* [](start = 58, length = 5)

This message is out-of-date

jcouv · 2018-11-15T00:01:43Z

src/Compilers/Core/Portable/InternalUtilities/StringTable.cs

-            return true;
-        }
+        internal static bool TextEquals(string array, ReadOnlySpan<char> text)
+            => text.Equals(array.AsSpan(), StringComparison.Ordinal);


text.Equals [](start = 15, length = 11)

I didn't understand how this .Equals works.
ReadOnlySpan.Equals seems to throw an exception (source )

https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/MemoryExtensions.Fast.cs#L37

(Spans can't be boxed anyway, so that method couldn't be the target)

jcouv · 2018-11-15T00:09:49Z

src/Compilers/Core/Portable/InternalUtilities/TextKeyedCache.cs

@@ -118,7 +118,7 @@ internal T FindItem(char[] chars, int start, int len, int hashCode)

            if (text != null && localSlot.HashCode == hashCode)
            {
-                if (StringTable.TextEquals(text, chars, start, len))
+                if (StringTable.TextEquals(text, chars.AsSpan(start, len)))


AsSpan [](start = 55, length = 6)

AsReadOnlySpan? (also below)

There's an implicit conversion, doesn't matter.

jcouv · 2018-11-15T00:12:32Z

I assume the benchmarks in the description were for a previous iteration with a broader change.

jcouv · 2018-11-15T00:13:27Z

src/Setup/DevDivInsertionFiles/DevDivInsertionFiles.csproj

    <ExpectedDependency Include="System.Runtime.CompilerServices.Unsafe" DevEnvLib="lib/netstandard2.0/System.Runtime.CompilerServices.Unsafe.dll"/>
    <ExpectedDependency Include="System.Text.Encoding.CodePages" DevEnvLib="lib/netstandard2.0/System.Text.Encoding.CodePages.dll"/>
+    <ExpectedDependency Include="System.Numerics.Vectors" DevEnvLib="lib/netstandard2.0/System.Numerics.Vectors.dll" />


System.Numerics.Vectors [](start = 33, length = 23)

Does this relate to System.Memory?

Dependent reference.

jcouv · 2018-11-15T00:14:05Z

src/Compilers/Core/CodeAnalysisTest/AnalyzerFileReferenceTests.cs

@@ -223,6 +223,8 @@ public class TestAnalyzer : DiagnosticAnalyzer

            dir.CopyFile(typeof(System.Reflection.Metadata.MetadataReader).Assembly.Location);
            dir.CopyFile(typeof(AppDomainUtils).Assembly.Location);
+            dir.CopyFile(typeof(Memory<byte>).Assembly.Location);


byte [](start = 39, length = 4)

nit: the byte is probably not needed: typeof(Memory<>) (here and in TestHelpers.cs)

jcouv

LGTM Thanks (iteration 3) with some clarifying questions

agocke added the Area-Compilers label Nov 14, 2018

agocke requested review from jaredpar and a team November 14, 2018 00:22

agocke requested a review from a team as a code owner November 14, 2018 00:22

CyrusNajmabadi reviewed Nov 14, 2018

View reviewed changes

agocke changed the base branch from master to dev16.0-preview2 November 14, 2018 00:29

Add System.Memory reference

3e3206e

Also replaces some unsafe code with Span<T>.

agocke force-pushed the add-memory-ref branch from ef844ba to 3e3206e Compare November 14, 2018 00:29

RikkiGibson approved these changes Nov 14, 2018

View reviewed changes

Therzok reviewed Nov 14, 2018

View reviewed changes

Therzok added a commit to mono/monodevelop that referenced this pull request Nov 14, 2018

[Span] Use span APIs to avoid unsafe

055c1ce

Mirrors dotnet/roslyn#31162

Therzok mentioned this pull request Nov 14, 2018

[Span] Use span APIs to avoid unsafe mono/monodevelop#6598

Merged

Add dependency DLLs to insertion list

56460d0

Therzok added a commit to mono/monodevelop that referenced this pull request Nov 14, 2018

[Span] Use span APIs to avoid unsafe (#6598)

53f0795

Mirrors dotnet/roslyn#31162

Copy new required net472 assemblies for analyzer tests

b7e3dc5

jcouv reviewed Nov 14, 2018

View reviewed changes

jcouv reviewed Nov 15, 2018

View reviewed changes

jcouv approved these changes Nov 15, 2018

View reviewed changes

jcouv self-assigned this Nov 15, 2018

jcouv added this to the 16.0.P2 milestone Nov 15, 2018

Respond to PR comments

38aac05

agocke merged commit db3a9f3 into dotnet:dev16.0-preview2 Nov 15, 2018

agocke deleted the add-memory-ref branch November 15, 2018 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add System.Memory reference #31162

Add System.Memory reference #31162

agocke commented Nov 14, 2018

CyrusNajmabadi Nov 14, 2018

Therzok Nov 14, 2018 •

edited

Loading

jcouv Nov 14, 2018

jcouv Nov 15, 2018 •

edited

Loading

agocke Nov 15, 2018

agocke Nov 15, 2018

jcouv Nov 15, 2018

jcouv Nov 15, 2018

agocke Nov 15, 2018

jcouv commented Nov 15, 2018

jcouv Nov 15, 2018

agocke Nov 15, 2018

jcouv Nov 15, 2018

jcouv left a comment

Add System.Memory reference #31162

Add System.Memory reference #31162

Conversation

agocke commented Nov 14, 2018

Choose a reason for hiding this comment

Therzok Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

Therzok Nov 14, 2018 •

edited

Loading

jcouv Nov 15, 2018 •

edited

Loading