Portable (and thus slower) version of Span<> · Pull Request #13562 · dotnet/corefx

ghost · 2016-11-10T21:57:57Z

Holding off on ReadOnlySpan<> until we're happy
with Span so I don't have keep making fixes in two
places.
Not too happy with the hoops DangerousGetPinnedReference
has to go through: we are looking at an alternative
that involves adding two new Unsafe apis:

https://github.com/dotnet/corefx/issues/13426

I'll have to do some benchmarking to see which one's faster,
though. In the meantime, there's plenty to review
that won't be affected by that.

ghost · 2016-11-10T21:59:30Z

justinvp · 2016-11-10T22:03:05Z

+        // As the name suggests, the offset is measured in bytes, not T's.
+        //
+        private readonly object _object;
+        private readonly IntPtr _byteOffset;


Nit: Vast majority of code in the repo has fields at the top. Can we do the same here?

Sorry, not a style I favor and not mandated by the guidelines.

@weshaggard / @stephentoub, thoughts?

It might be not mandated by the guidelines, but it might be better to be consistent with the rest of the repo (and add a guideline :-))

I would like them to be at the top. It's how we do it basically everywhere else in the repo, and where we don't it's effectively a style bug we fix. If it's not in the guidelines, it should be.

omariom · 2016-11-10T22:20:28Z

I think it is important to have the indexer, ctors and Slice methods inlined.

ghost · 2016-11-10T22:24:54Z

I think it is important to have the indexer, ctors and Slice methods inlined.

Wasn't sure they'd inline with all the parameter validation code but sure, I can always put the MethodImpl attribute on them.

omariom · 2016-11-10T22:32:17Z

Inlining should be checked on the older runtimes as the current JIT is different (more capable).

jkotas · 2016-11-10T23:10:04Z

+        /// </summary>
+        public static bool IsReferenceFree<T>()
+        {
+            if (PerTypeLatches<T>.IsReferenceFree)


This should be just PerTypeLatches<T>.IsReferenceFree and the initialization should done via static constructor in PerTypeLatches

adamsitnik · 2016-11-10T23:16:03Z

+
+        /// <summary>
+        /// Returns a cached empty array (Array.Empty not available on S.R. 4.0.0.0)
+        /// </summary>


It seems to return new array every time, not cached as stated in the comment

Oops - yeah - last minute code thrown in in the migrate to corefx...

justinvp · 2016-11-10T23:25:43Z

+        /// <summary>
+        /// The number of items in the span.
+        /// </summary>
+        public int Length { get; }


A getter-only property is fine to use here, but I wonder if it'd be better for this particular struct to have an explicit readonly _length field (declared along with the other fields), to make it more clear to readers what the size of the struct is.

Seems reasonable.

stephentoub · 2016-11-10T23:25:46Z

@@ -0,0 +1,35 @@
+// Licensed to the .NET Foundation under one or more agreements.


What does the ".Microsoft" in the filename refer to? If it's about a particular platform, we should use a word or words that best describes the target.

Microsoft .NET runtimes (CLR+CoreClr vs. as opposed to say, Mono.)

Mono is Microsoft runtime too...

Hence the difficulty in choosing a good name - with the "store as Pinnable" trick, though, we can mothball this file altogether and have a implementation that works on Mono too. So this is moot.

stephentoub · 2016-11-10T23:28:17Z

+            if (array == null)
+                throw new ArgumentNullException(nameof(array));
+            if (default(T) == null && array.GetType() != typeof(T[]))
+                throw new ArrayTypeMismatchException(SR.Format(SR.ArrayTypeMustBeExactMatch, typeof(T)));


I believe there were some recent JIT improvements around conditionalized exception throwing like this regarding inlining, but even with that, should we separate out these throws into their own methods?

JIT improvements around conditionalized exception throwing

Note that this implementation is primarily meant for existing full framework runtimes that do not have these improvements.

should we separate out these throws into their own methods?

Yes

stephentoub · 2016-11-10T23:32:24Z

+            if ((uint)Length > (uint)destination.Length)
+                return false;
+
+            // TODO: This is a tide-over implementation as we plan to add a overlap-safe cpblk-based api to Unsafe.


Is there an issue # we can add to the TODO?

https://github.com/dotnet/corefx/issues/13427

KrzysztofCwalina · 2016-11-10T23:35:18Z

+        /// <exception cref="System.ArgumentOutOfRangeException">
+        /// Thrown when the specified <paramref name="length"/> is negative.
+        /// </exception>
+        public static Span<T> DangerousCreate(object obj, ref T rawPointer, int length)


Did we decide for the second parameter to be ref T or IntPtr? I honestly don't remember. If we decided for ref T, do you remember why?

It's ref, because a ref T is easy to come up with.

"ref T" is how it ended up in the final ApiReview:

https://github.com/dotnet/apireviews/tree/master/2016/11-04-SpanOfT

stephentoub · 2016-11-10T23:35:27Z

+        ///     Start and index are non-negative, and already pre-validated to be within the valid range of their containing Span.
+        ///     
+        ///     If the byte length (Span.Length * sizeof(T)) does an unsigned overflow (i.e. the buffer wraps or is too big to fit within the address space),
+        ///     the behavior is undefined.


Should we add a Debug.Assert for it just to help catch some potential misuse?

I have asserts for start and index being negative (as those are already validated.)

The unsigned overflow is unvalidated at this point and they come from the application so it's not a condition I can assert on.

so it's not a condition I can assert on

Are there valid uses cases where it would overflow?

They're probably not valid, per se, (an app would be asking for a Span that wraps the address space), but usually we reserve asserts to detect conditions that are the framework's fault, not the app's fault.

So if it's possible for an app to get data to this internal helper that's invalid, shouldn't there be an exception to catch that earlier in the call path? And if there is, then we can assert here.

The only way an unsigned overflow can get here are the two unsafe constructions (.ctor(void*) and DangerousCreate()).

Adding parameter validation to those power routines would probably not go down well around these parts.

I'd be open to adding the assert to the safe constructors with arrays if I could find an unobstrusive way to code it. But between the 32-64 bit dichotomy and all the restrictions on IntPtr arithmetic, such an assert would end up overwhelming the code and it's a really unlikely case since it would mean managed array construction is unsafe.

The only way an unsigned overflow can get here are the two unsafe constructions (.ctor(void*) and DangerousCreate()).

Ok

stephentoub · 2016-11-10T23:37:57Z

+
+        private static bool IsReferenceFree(Type type)
+        {
+            if (type.GetTypeInfo().IsPrimitive) // This is hopefully the common case. All types that return true for this are value types w/out embedded references.


Should we add a check for IsEnum?

KrzysztofCwalina · 2016-11-10T23:38:11Z

+                if (srcGreaterThanDst)
+                {
+                    // Source address greater than or equal to destination address. Can do normal copy.
+                    for (int i = 0; i < Length; i++)


Why don't we do a bulk copy here?

See the TODO a few lines up.

stephentoub · 2016-11-10T23:40:28Z

+    <AssemblyVersion>4.0.3.0</AssemblyVersion>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <CLSCompliant>false</CLSCompliant>
+    <NuGetTargetMoniker>.NETStandard,Version=v1.0</NuGetTargetMoniker>


We should add:

<DocumentationFile>$(OutputPath)$(AssemblyName).xml</DocumentationFile>

so that all of your good docs end up getting validated and generated into a docs file.

jkotas · 2016-11-10T23:43:32Z

+        {
+            get
+            {
+                switch (_object)


This is super slow. Instead, this should be something like:

public struct Span<T> { [StructLayout(LayoutKind.Sequential)] private sealed class Pinnable { public T Data; } private readonly Pinnable _pinnable; private readonly IntPtr _byteOffset; public ref T DangerousGetPinnableReference { get { return (_pinnable != null) ? Unsafe.AddByteOffset<T>(ref _pinnable.Data, _byteOffset) : Unsafe.AsRef<T>((void*)_byteOffset); } } }

Also, I would manually inline the line into the indexer so that the JIT has less inlining to do for it.

This will cause us to take a ref of the address within the array header (i.e. the hidden "length+padding" "field"). You may recall I asked a couple of days ago if that was safe to do.

I do not recall this question being asked ... It is safe to do on .NET Framework/.NET Core runtimes that this implementation seems to be specific to anyway.

With this trick, it might become less specific as we can avoid hard-coding the Mono layout. But let's go with this for now.

How does this trick work?

Won't this be broken for arrays?

Yeah I couldn't understand why the @jkotas version should not be possible, see https://github.com/dotnet/corefx/issues/13426#issuecomment-259913759 This also means we do not need extra overloads for Unsafe.

joshfree · 2016-11-10T23:44:28Z

/cc @davidfowl

jkotas · 2016-11-10T23:46:05Z

+                if (srcGreaterThanDst)
+                {
+                    // Source address greater than or equal to destination address. Can do normal copy.
+                    for (int i = 0; i < Length; i++)


Cache Length into local for better perf?

KrzysztofCwalina · 2016-11-10T23:47:42Z

+        }
+
+        [Fact]
+        public static void Overlapping2()


Why are you not using [Theory] instead of having Overlapping1, Overlapping2, ...?

Partly because I wrote all this in a desktop infrastructure where I have only a minimal XUnit emulator.

And partly because I frankly don't care for the [Theory] construct. I like unit tests that I can drag out to a simple standalone app for me to study when they break.

Theories are much better for exactly this reason though though 😄 . Same logical test with different data sets.

Also, i don't think not using theories scales past span. Many other APIs that we will be adding (e.g. formatting) are so much more complicated than spans that we won't be able to avoid using theories.

KrzysztofCwalina · 2016-11-10T23:50:17Z

+            int[] src = { 1, 2, 3 };
+            int[] dst = { 99, 100, 101 };
+
+            Span<int> srcSpan = new Span<int>(src);


How are we going to ensure that the fast-span and slow-span are tested? i.e. the tests run in the context of coreclr, which has the fast span.

I believe that's done by building a facade for CoreClr. It's not something that can be right now until the CoreClr implementation is brought up to date with the spec.

BTW: It would be fine to do the reference assembly and CoreCLR façade plumbing right away. Most of the APIs are in CoreCLR already, and it is easy to add baseline for the few APIs that are not there yet.

davidfowl · 2016-11-10T23:46:51Z

+        /// <exception cref="System.ArgumentOutOfRangeException">
+        /// Thrown when the specified <paramref name="length"/> is negative.
+        /// </exception>
+        public static Span<T> DangerousCreate(object obj, ref T rawPointer, int length)


It's ref, because a ref T is easy to come up with.

davidfowl · 2016-11-10T23:47:43Z

+            // Disallow "special" objects that have their own object headers and would yield unexpected (and runtime-dependent) results.
+            // We already have constructors for arrays (and that enforce the passing of the properly typed single-dimensional array.)
+            // This constructor is not useful for strings (cannot express"ref s[i]").
+            if (obj is Array)


We're explicitly blocking these? Why?

string: an immutable type being wrapped inside a mutable Span... and you can't take a "ref s[0]" so there's no correct way to use it anyway.

array: this boils down to - do we want to allow multidim arrays or not? (what guarantees do we make about the layout of those (row order or column order?) We could allow T[] to go through but that's another check and it duplicates functionality that we have more strongly typed constructors for.

The offered up rationale for these were plain old objects with "fixed array-like stuff in them" so that's what I'm scoping these to.

We should not be in the business of enforcing what kind of stuff can or cannot be used with the DangerousCreate. This API is for very advanced users that know what they are doing:

It costs perf. The advanced users call this API for perf. Wasting perf for unnecessary checks makes this API useless.

The advanced users always find a valid reasons for why they want to do something, and these unnecessary checks always get in their way.

Yes, we definitely don't want all these checks in. And calling ref T rawPointer seems wrong to me, it is not a pointer, it is a ref. The description of this parameter makes this clear, "A reference to data within that object", so call it objectData the ref is clear from the type. It is not a raw pointer at all.

davidfowl · 2016-11-10T23:52:57Z

+        /// <exception cref="System.IndexOutOfRangeException">
+        /// Thrown when index less than 0 or index greater than or equal to Length
+        /// </exception>
+        public ref T this[int index]


This is correct but none of the tooling outside of corefx supports ref returns. I think we need to return a T for now with a setter and a TODO to unblock this once the tooling is far along enough. I believe these methods would be filtered out if using C#6

It's not so easy to change after it's out in the wild - it's a breaking change and things that depend on it will have to be recompiled at least. I believe the decision was that we'd accept C#7 being a prereq.

We shouldn't ship this way (to nuget.org). I'm talking about a few months. Maybe we'll have to expedite that somehow..... Im worried about being able to use this immediately (right after this goes in).

What prevents you from picking up latest Roslyn beta for your experiments? It is in the current dotnet CLI builds; it is easy to add it to .csproj file if you are using classic VS projects; ... .

We have even said that C# 8 will be eventual prereq for Span to get enforcement that it is used correctly.

xproj and project.json. Everything else is using that and we'd need to make a new build with C# 7 which nobody is looking at. The next version of Visual Studio will have proper support.... but I'd like to use it with VS2015 and xproj since all of the existing .NET Core tooling is based on that.

Yes, absolutely, have you tried it? 😄 There is no VS2015 support...

They use VSCode/Sublime on the CLI itself... I actually like using visual studio

installing preview3 currently breaks VS 2015 (or exposes a bug in VS depending how you look at it) - and I think VS 2015 will need a patch to work; with Core, when it is installed.

e.g. you can't File->New to get a working project and need to do all restores from command line https://github.com/dotnet/cli/issues/4248#issuecomment-259551612

@davidfowl - the C# workaround is in. If it meets with your approval, pls sign off (or better yet, hit the merge button.)

davidfowl · 2016-11-11T01:05:15Z

@atsushikan You should avoid merging commits during review. This PR is now unreadable as it has 323 changed files...

ghost · 2016-11-11T01:08:47Z

Unfortunately, these frequent version bumps make it necessary - otherwise, CI's become useless.

davidfowl · 2016-11-11T01:10:39Z

stephentoub · 2016-11-11T01:10:52Z

Unfortunately, these frequent version bumps make it necessary - otherwise, CI's become useless.

You can rebase instead of merging.

davidfowl · 2016-11-11T04:02:54Z

+<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <ItemGroup>
+    <Project Include="System.Memory.csproj" Condition="'$(OS)'=='Windows_NT'" />


Why is this windows only? (I have no idea how the corefx builds work)

Because S.R.C.Unsafe is Windows only. No, I don't know why.

S.R.CS.Unsafe is built on Windows only. I suspect that it is because of ilproj support was not available in Unix buildtools at the time it was introduced. (It is no longer the case so it should be possible to remove this condition even for S.R.CS.Unsafe.)

System.Memory does not need ilasm to build. It uses S.R.CS.Unsafe from package so I do not think there is good reason for having this condition here.

What's the matter, don't we have any version numbers you like??

(all this repeated source is annoying - unfortunately, Rosylin at this point at least, insists that "ref" variables be initialized once only at declaration point and doesn't allow the conditional ternary operator on ref types. This makes it hard to manually inline DangerousGetPinnableReference in a nice way...)

Allow JIT to recognize the throw path as a no-return path so it can throw it into the cold region.

It pessimizes the routine on the desktop CLR.

Description: "Provide classes" => "Provide types" Add fastpath for specific types in IsReferenceFree Make MeasureArrayAdjustment a PerTypeValues private

Eh... hard to tell with all the noise but it did seems to shave a couple of percentage points off.

Ah, the fun of writing Span code w/out stack-only enforcement.

...to typeforward on CoreClr. Right now, CoreClr fails 44 out of 55 tests so I believe it's premature to throw the switch just yet. This just gets some of the grunt work out of the way.

davidfowl · 2016-11-15T09:53:57Z

+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <OutputType>Library</OutputType>
+    <CLSCompliant>false</CLSCompliant>
+    <AssemblyVersion>4.0.0.0</AssemblyVersion>


Why is this 4.0.0.0?

What else would it be?

After chatting offline, we'll accomodate C# 6.0 for now by having the indexer return "T" rather than "ref T". Some performant code require the "ref T" version so as another stopgap, we'll provide a GetItem() method that returns "ref T". Once the tooling story is in better shape, we'll merge GetItem() and the indexer into a single indexer that returns "ref T".

This is getting really monotonous.

KrzysztofCwalina · 2016-11-15T20:02:59Z

+        // TODO: https://github.com/dotnet/corefx/issues/13681
+        //   Until we get over the hurdle of C# 7 tooling, this indexer will return "T" and have a setter rather than a "ref T". (The doc comments
+        //   continue to reflect the original intent of returning "ref T")
+        public T this[int index]


Looks good. Thanks!

KrzysztofCwalina · 2016-11-15T20:05:25Z

I think it's ready to merge after the tests pass.
Ati, thanks for all the good work!

dnfclas added the cla-already-signed label Nov 10, 2016

justinvp reviewed Nov 10, 2016

View reviewed changes

jkotas reviewed Nov 10, 2016

View reviewed changes

adamsitnik reviewed Nov 10, 2016

View reviewed changes

justinvp reviewed Nov 10, 2016

View reviewed changes

stephentoub reviewed Nov 10, 2016

View reviewed changes

KrzysztofCwalina reviewed Nov 10, 2016

View reviewed changes

stephentoub reviewed Nov 10, 2016

View reviewed changes

KrzysztofCwalina reviewed Nov 10, 2016

View reviewed changes

stephentoub reviewed Nov 10, 2016

View reviewed changes

jkotas reviewed Nov 10, 2016

View reviewed changes

KrzysztofCwalina reviewed Nov 10, 2016

View reviewed changes

davidfowl reviewed Nov 11, 2016

View reviewed changes

karelz added the area-System.Runtime label Nov 11, 2016

karelz assigned ghost Nov 11, 2016

davidfowl reviewed Nov 11, 2016

View reviewed changes

Atsushi Kanamori added 19 commits November 14, 2016 13:02

IsReferenceFree fast-paths for common primitives.

f25a42a

Move exception throwing out of inline code.

c47176c

Remove Windows-only restriction.

9f9665d

Remove baseline version.

79c4f3f

Don't hardcode array header size.

bde890e

Another dependency version change!

1c400cf

What's the matter, don't we have any version numbers you like??

Version => 4.0.0.0

7817577

ThrowHelper.Throw ==> throw ThrowHelper.Create

b4c7015

Allow JIT to recognize the throw path as a no-return path so it can throw it into the cold region.

Revert the specific primitive type checks in IsReferenceFree().

7ee7dcf

It pessimizes the routine on the desktop CLR.

Fix DangerousGetPinnableReference to be a method to match spec.

1ac3979

PR feedback.

dce1728

Description: "Provide classes" => "Provide types" Add fastpath for specific types in IsReferenceFree Make MeasureArrayAdjustment a PerTypeValues private

More concise operator==

3155862

Eh... hard to tell with all the noise but it did seems to shave a couple of percentage points off.

Follow Ben's suggested ThrowHelper pattern.

3334c94

Update ThrowHelper comments based on Ben's feedback.

e4fa8ac

Remove trailing whitespace.

4cfbba9

Dependency bump. Again...

bdbaf59

Fix up some tests so the IL is Span-legal.

2850306

Ah, the fun of writing Span code w/out stack-only enforcement.

Add a reference assembly and some of the plumbing...

a65d0b4

...to typeforward on CoreClr. Right now, CoreClr fails 44 out of 55 tests so I believe it's premature to throw the switch just yet. This just gets some of the grunt work out of the way.

davidfowl reviewed Nov 15, 2016

View reviewed changes

Atsushi Kanamori added 2 commits November 15, 2016 11:54

Dependency update again.

d551c71

This is getting really monotonous.

KrzysztofCwalina reviewed Nov 15, 2016

View reviewed changes

stephentoub merged commit ccc44b7 into dotnet:master Nov 15, 2016

ghost deleted the span branch November 15, 2016 21:28

karelz modified the milestone: 1.2.0 Dec 3, 2016

karelz added area-System.Memory and removed area-System.Runtime labels Feb 24, 2017

		@@ -0,0 +1,35 @@
		// Licensed to the .NET Foundation under one or more agreements.

Conversation

ghost commented Nov 10, 2016

Uh oh!

ghost commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrzysztofCwalina Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omariom commented Nov 10, 2016

Uh oh!

ghost commented Nov 10, 2016

Uh oh!

omariom commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrzysztofCwalina Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

KrzysztofCwalina Nov 10, 2016 •

edited

Loading

KrzysztofCwalina Nov 10, 2016 •

edited

Loading