Merge SpanOfT branch into master #7886

jkotas · 2016-10-29T02:23:45Z

Part of https://github.com/dotnet/coreclr/issues/5851

jkotas · 2016-10-29T02:25:17Z

@atsushikan PTLA

FYI @adamsitnik We are getting closer to make Span real. Thank you for your work on it!

jkotas · 2016-10-29T02:26:39Z

cc @joshfree

jkotas · 2016-10-29T02:29:12Z

The tests are not enabled in automation because of the Span is not exposed in the contracts yet.

davidfowl · 2016-10-29T08:31:19Z

src/mscorlib/src/System/ReadOnlySpan.cs

+        }
+    }
+
+    public static class ReadOnlySpanExtensions


Should we kill these extension methods until we get more clarity on Span/Memory sting interactions?

There is work to do to come up with the final version of the Span APIs, and then fix up both Span implementations to it. I do not think that it is productive to do speculative tweaks until then.

(And these extensions are not affected by Span/Memory interactions. They may be affected by slicing language syntax though.)

One thing we could do is leave the extension methods in corfxlab. I will be easier/faster to tweak them.

We can, but we would be losing some perf - number of the current extension methods are implemented using constructs that are not in the public surface.

MartinJohns

Why not document all public APIs?

MartinJohns · 2016-10-29T16:55:14Z

src/mscorlib/src/System/ReadOnlySpan.cs

+        public T[] ToArray()
+        {
+            var destination = new T[_length];
+            TryCopyTo(destination);


What if it fails? A Try pattern implies it can fail and a success value is returned, which is not used here. I'd expect it to be either used or have an inline comment if it's sure that it can't fail.

jkotas · 2016-10-30T09:33:49Z

@MartinJohns Thanks for taking a look - I have incorporated your feedback.

MartinJohns · 2016-10-30T11:20:00Z

src/mscorlib/src/System/ReadOnlySpan.cs

+    public struct ReadOnlySpan<T>
+    {
+        /// <summary>A byref or a native ptr. Do not access directly</summary>
+        internal /* readonly */ IntPtr _rawPointer;


Why is the readonly commented out?

It was necessary for the JitHelpers that fetch the field as ref. These JitHelpers are not 100% correct anyway wrt GC and we need work in the JIT to fix it - I have removed them and switched the fields to private readonly.

stephentoub · 2016-10-30T19:05:04Z

src/mscorlib/src/mscorlib.txt

@@ -631,6 +631,10 @@ Argument_UnmanagedMemAccessorWrapAround = The UnmanagedMemoryAccessor capacity a
 Argument_UnrecognizedLoaderOptimization = Unrecognized LOADER_OPTIMIZATION property value.  Supported values may include "SingleDomain", "MultiDomain", "MultiDomainHost", and "NotSpecified".
 ArgumentException_NotAllCustomSortingFuncsDefined = Implementations of all the NLS functions must be provided.
 ArgumentException_MinSortingVersion = The runtime does not support a version of "{0}" less than {1}.
+#if FEATURE_SPAN_OF_T
+Argument_InvalidTypeWithPointersNotSupported = Cannot use type '{0}'. Only value types without pointers are supported.


Nit: We should probably clarify the error message to call out that only value types without pointers or references are supported, since they show up differently in the language.

Some of the doc comments also contain the text "type may not contain pointers."

And how does runtime enforcement of this (JitHelpers.ContainsGcReferences) happen on the OOB version?

stephentoub · 2016-10-30T19:06:43Z

src/mscorlib/src/System/Span.cs

+namespace System
+{
+    /// <summary>
+    /// Span is a uniform API for dealing with arrays and subarrays, strings


Nit: in its current form, it's not really for strings and substrings

I am replacing this description with the one in @KrzysztofCwalina spec.

stephentoub · 2016-10-30T19:20:57Z

src/vm/methodtablebuilder.cpp

+        else if (strcmp(name, g_ReadOnlySpanName) == 0)
+        {
+            pMT->SetIsByRefLike();
+        }


Is this going to cause any problems if someone has their own System.Span<T> or System.ReadOnlySpan<T> defined in their own library?

This only runs for types in corelib. The callsite of this method is under if (GetModule()->IsSystem()).

OK, let's leave them here as-is. We want to get quickly to a point where we understand where we are with perf. We can tweak the APIs in both places and sync them periodically.

- Generic ArrayPinningHelper resulted into incorrect array data offset for reference types. Use non-generic one instead. - Add array covariance checks to guarantee type safety.

…otnet#7220) * added AsBytes() and NonPortableCast() to Span and ReadOnlySpan API, fixes #7211 * throw when casted types contain pointers

Fix missing pinning in CopyTo<T> Move and rename some JitHelpers to S.R.CS.Unsafe to make different implementations more similar

KrzysztofCwalina · 2016-10-31T15:37:33Z

src/mscorlib/src/System/ReadOnlySpan.cs

+        /// Thrown when the specified <paramref name="length"/> is negative.
+        /// </exception>
+        [CLSCompliant(false)]
+        public unsafe ReadOnlySpan(void* ptr, int length)


could the parameter be called "pointer"? It's more aligned with out naming conventions and I use Pointer for the same concept in in Memory APIs .

KrzysztofCwalina · 2016-10-31T15:52:49Z

tests/src/CoreMangLib/system/span/BasicSpanTest.cs

@@ -0,0 +1,672 @@
+using System;


@atsushikan, it would be great if fast and slow span tests were equivalent to ensure the same semantics.

ghost · 2016-10-31T15:54:38Z

src/mscorlib/src/System/ReadOnlySpan.cs

+        {
+            if (array == null)
+                ThrowHelper.ThrowArgumentNullException(ExceptionArgument.array);
+            if ((uint)start >= (uint)array.Length || (uint)length > (uint)(array.Length - start))


This check makes this illegal:

int[] a = new int[5]; ReadOnlySpan<int> span = new ReadOnlySpan<int>(a, 5, 0);

It should be valid (and consistent with other api's that work on a segment of an array) to take an empty span starting from "one past" the end of the array.

ghost · 2016-10-31T16:35:03Z

Also, ReadOnlySpan.Slice(this String text, ...) has the too strict range check.

jkotas · 2016-10-31T16:49:05Z

ReadOnlySpan.Slice(this String text, ...) has the too strict range check

Fixed.

dnfclas added the cla-already-signed label Oct 29, 2016

jkotas force-pushed the SpanOfT branch from 2667eb7 to 17ac043 Compare October 29, 2016 05:32

davidfowl reviewed Oct 29, 2016

View reviewed changes

jkotas force-pushed the SpanOfT branch 2 times, most recently from b4fe60d to d0ebbd4 Compare October 29, 2016 16:57

MartinJohns reviewed Oct 29, 2016

View reviewed changes

MartinJohns reviewed Oct 30, 2016

View reviewed changes

stephentoub reviewed Oct 30, 2016

View reviewed changes

jkotas force-pushed the SpanOfT branch from 7eec32e to 8fcc6af Compare October 30, 2016 22:00

jkotas and others added 15 commits October 30, 2016 17:26

Add FEATURE_SPAN_OF_T

d28b09f

Use Roslyn with ref returns and locals

096020e

Span support in VM

cb17e33

Span<T> managed implementation

d5d5f8c

Add implementation for hand-written JIT helpers

24a8d61

Add basic Span<T> test

9c7c1b7

Fix reference types (dotnet#6954)

e7dfda5

- Generic ArrayPinningHelper resulted into incorrect array data offset for reference types. Use non-generic one instead. - Add array covariance checks to guarantee type safety.

Add TryCopyTo and Set methods to Spans API #5857 (dotnet#6648)

3ba437e

added AsBytes() and NonPortableCast() to Span and ReadOnlySpan API (d…

93ee105

…otnet#7220) * added AsBytes() and NonPortableCast() to Span and ReadOnlySpan API, fixes #7211 * throw when casted types contain pointers

create ReadOnlySpan<char> from string (dotnet#7276)

59f652e

added implicit cast operator from ArraySegment to Spans (dotnet#7297)

ca90acb

Disable Span<T> GC reporting under TODO to workaround JIT issue

94362ac

Fix build break on 32-bit platforms

30af1dc

Streamline implementation of ToArray to call CopyTo<T> directly

c718ac4

Fix missing pinning in CopyTo<T> Move and rename some JitHelpers to S.R.CS.Unsafe to make different implementations more similar

Add boilerplate doc comments to trivial methods

b1ffa42

jkotas force-pushed the SpanOfT branch 2 times, most recently from 1adfa90 to f31afb2 Compare October 31, 2016 03:55

KrzysztofCwalina reviewed Oct 31, 2016

View reviewed changes

ghost reviewed Oct 31, 2016

View reviewed changes

jkotas force-pushed the SpanOfT branch from 1df140c to 6496fde Compare October 31, 2016 16:34

jkotas added 2 commits October 31, 2016 09:47

Change Span fields to private readonly + other review feedback

c9a4811

Add workaround for JIT crash

4c5e979

jkotas force-pushed the SpanOfT branch from 6496fde to 4c5e979 Compare October 31, 2016 16:47

jkotas merged commit f429841 into dotnet:master Oct 31, 2016

jkotas deleted the SpanOfT branch November 2, 2016 16:57

karelz modified the milestone: 2.0.0 Aug 28, 2017

jkotas mentioned this pull request May 3, 2018

Expose a Utf8String type. #17872

Closed

jkotas mentioned this pull request Jan 31, 2020

Span<T> dotnet/runtime#6168

Closed

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge SpanOfT branch into master #7886

Merge SpanOfT branch into master #7886

jkotas commented Oct 29, 2016

jkotas commented Oct 29, 2016

jkotas commented Oct 29, 2016

jkotas commented Oct 29, 2016

davidfowl Oct 29, 2016

jkotas Oct 29, 2016

KrzysztofCwalina Oct 31, 2016

jkotas Oct 31, 2016

MartinJohns left a comment

MartinJohns Oct 29, 2016

jkotas commented Oct 30, 2016

MartinJohns Oct 30, 2016

jkotas Oct 30, 2016

stephentoub Oct 30, 2016

jkotas Oct 30, 2016

ghost Oct 31, 2016

stephentoub Oct 30, 2016

jkotas Oct 30, 2016 •

edited

Loading

stephentoub Oct 30, 2016

jkotas Oct 30, 2016

KrzysztofCwalina Oct 31, 2016

KrzysztofCwalina Oct 31, 2016

jkotas Oct 31, 2016

KrzysztofCwalina Oct 31, 2016

ghost Oct 31, 2016

jkotas Oct 31, 2016

ghost commented Oct 31, 2016

jkotas commented Oct 31, 2016

Merge SpanOfT branch into master #7886

Merge SpanOfT branch into master #7886

Conversation

jkotas commented Oct 29, 2016

jkotas commented Oct 29, 2016

jkotas commented Oct 29, 2016

jkotas commented Oct 29, 2016

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

MartinJohns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Oct 30, 2016

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

jkotas Oct 30, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 31, 2016

jkotas commented Oct 31, 2016

jkotas Oct 30, 2016 •

edited

Loading