Add TryCopyTo and Set methods to Spans API #5857 by adamsitnik · Pull Request #6648 · dotnet/coreclr

adamsitnik · 2016-08-08T10:05:01Z

Hello!

@jkotas @KrzysztofCwalina Could you review the code and tell if everything is ok before I implement Set method and copy the code for ReadOnlySpan?

Jan I have seen that you added some basic tests for Span, but it seems that they are not executed during build with tests included. Could you add it to the SpanOfT branch or tell me how can I quickly execute this tests?

omariom · 2016-08-08T12:02:29Z

src/vm/jitinterface.cpp

+
+        BYTE resultIlCode;
+        if (CorTypeInfo::IsObjRef(elementType) || (elementType == ELEMENT_TYPE_VALUETYPE && typeHandle.GetMethodTable()->ContainsPointers()))
+        {


Is seems to ignore primitives like ELEMENT_TYPE_I4.
I think the whole check should be just for ContainsPointers.

@omariom I am not sure. I think that it will execute the else case in that scenario and will always return false.

I've confused the branches )
But what about just ContainsPointers? Isn't it enough?

I think the best way to write is:

MethodTable * pMT = typeHandle.GetMethodTable(); if (!pMT->IsValueType() || pMT->ContainsPointers()) { ... }

But what about just ContainsPointers?

It does not work for non-valuetype types. E.g. class Foo { int x; }. MethodTable::ContainsPointers returns false, but we want this primitive to return true.

It may be worth adding a comment explaining difference between MethodTable::ContainsPointers and ContainsReferences.

jkotas · 2016-08-08T13:34:09Z

tell me how can I quickly execute this tests?

For now, I have been running it manually:

d:\coreclr\bin\Product\Windows_NT.x64.Debug>csc /noconfig /nostdlib /r:System.Private.CoreLib.dll \coreclr\tests\src\CoreMangLib\system\span\BasicSpanTest.cs

d:\coreclr\bin\Product\Windows_NT.x64.Debug>corerun BasicSpanTest.exe

Yes, there is a work to do in getting hooked up in the regular test runs. I have not done it because of the tests are compiled against contracts by default. Since Span is not in contracts yet, it would require some special sauce to make it work.

jkotas · 2016-08-08T13:36:48Z

@adamsitnik Thank you for getting this started! It looks great to me, modulo comments.

adamsitnik · 2016-08-09T15:36:57Z

@jkotas Thanks for all the comments! Today I added some detailed tests. Currently all of them fail, will investigate tomorrow.

jamesqo · 2016-08-11T00:47:55Z

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

    static void Main()
+    {
+        Test(() => CanAccessItemsViaIndexer(), "CanAccessItemsViaIndexer");
+


Use nameof here?

…ypeCheck does

adamsitnik · 2016-08-27T20:12:36Z

@jkotas I got stucked again. I have debugged the CLR but without luck.

The problem: it seems that slices do not work well with the reference types even without my changes. Sample failing test:

class ReferenceType
{
    internal byte Value;
    public ReferenceType(byte value) { Value = value; }
}

private static void ReferenceTypesAreSupported()
{
    var underlyingArray = new ReferenceType[] { new ReferenceType(0), new ReferenceType(1), new ReferenceType(2) };
    var slice = new Span<ReferenceType>(underlyingArray);

    for (int i = 0; i < underlyingArray.Length; i++)
    {
        AssertTrue(underlyingArray[i].Value == slice[i].Value, "Values are different");
        AssertTrue(object.ReferenceEquals(underlyingArray[i], slice[i]), "References are broken");
    }
}

result: "test has failed with exception: Object reference not set to an instance of an object."

Jan could you take a look at this? This is blocking for me because I am not sure if my copying tests fail because of general Slice issue or my code has a bug.

omariom · 2016-08-27T21:08:08Z

@adamsitnik
what code Slice indexer getter generates here?

adamsitnik · 2016-08-28T11:00:02Z

@omariom This is tricky ;)

Slice indexer getter:
JitHelpers.AddByRef(ref JitHelpers.GetByRef<T>(ref _rawPointer), index);

JitHelper.cs constains the declarations:

static internal ref T GetByRef<T>(ref IntPtr byref)
static internal void SetByRef<T>(out IntPtr byref, ref T value)
static internal ref T AddByRef<T>(ref T pointer, int count)
static internal bool ByRefEquals<T>(ref T refA, ref T refB)

Which are replaced by the actual CIL at runtime by CLR with help of jitinterface.cpp.

GetByRef is CEE_LDARG_0, CEE_LDIND_I, CEE_RET
AddByRef is

CEE_LDARG_1, 
CEE_LDC_I4 size,
CEE_CONV_I, 
CEE_MUL, 
CEE_LDARG_0, 
CEE_ADD, 
CEE_RET

jkotas · 2016-08-29T06:17:16Z

Thank you for tracing down the bug in handling of reference types. I have fixed it in #6954.

adamsitnik · 2016-08-29T09:19:43Z

@jkotas Thanks for such fast response! Everytime I read your commits I learn something new.

Should I adopt my SizeOf() to the new way that you have implemented by using the metadata token and
CEE_SIZEOF; instr? Previously I had no idea how to obtain !!T

jkotas · 2016-08-29T14:20:11Z

Should I adopt my SizeOf() to the new way that you have implemented by using the metadata token and CEE_SIZEOF?

I think so. The previous implementation was not actually thread safe. The IL is in static variable - it is not ok for different threads to end up with different IL.

jkotas · 2016-08-29T14:23:07Z

For same reason, ContainsReferences should be done by having two static const byte arrays - one for returning false and one for returning true.

Conflicts: src/mscorlib/src/System/Runtime/CompilerServices/jithelpers.cs src/vm/jitinterface.cpp src/vm/mscorlib.h tests/src/CoreMangLib/system/span/BasicSpanTest.cs

…whole loop, it did not work well with structs with pointers

adamsitnik · 2016-08-29T19:09:19Z

@jkotas This commit has fixed my test for copying value types with reference. Now all tests are passing!

Could you do one more code review?

Then I simply copy all the changes to ReadOnlySpan and we should be able to merge.

jkotas · 2016-08-29T20:51:17Z

This commit has fixed my test for copying value types with reference.

The original code should work fine. Is this commit working around a bug?

jkotas · 2016-08-29T20:55:32Z

src/mscorlib/src/System/Span.cs

+            get { return _length; }
+        }
+
+        internal uint SizeInBytes


Using uint for byte sizes is problematic because it can overflow on 64-bit platforms. You may want to use nuint like here: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Buffer.cs#L18

Conflicts: src/mscorlib/src/System/Span.cs

adamsitnik · 2016-08-30T14:24:37Z

Actually, the original code had a bug - it was de-referencing the managed pointer. It is necessary to sprinkle ref throughout to make this work like this:

Thanks for the explanation!

I added unified contracts for all methods. The other minor (change) was to add the Slice(System.Int32) implementation as requested in #5857. I think that the code is ready,

jkotas · 2016-08-30T14:30:07Z

src/mscorlib/src/System/Span.cs

+    internal static class SpanContracts
+    {
+        internal static void RequiresIndexInRange(int index, int length) {
+            if ((uint)index >= (uint)length)


I do not think that having contract methods on the side is the preferred corefx coding style. We have done experiments with code contracts in the past, so some old code may still look similar to this ... but it was abandoned as not a very good idea.

@jkotas Could you provide me a link with a sample that has contracts implemented in the right way?

Regular if-checks. Like it used to be before 496e344

jkotas · 2016-08-30T16:39:46Z

LGTM modulo last few comments. @KrzysztofCwalina Could you please take a look as well?

KrzysztofCwalina · 2016-08-30T17:05:32Z

src/mscorlib/src/System/ReadOnlySpan.cs

+        /// <param name="destination">An unmanaged pointer to memory.</param>
+        /// <param name="elementsCount">The number of T elements the memory contains.</param>
+        [CLSCompliant(false)]
+        public unsafe bool TryCopyTo(void* destination, int elementsCount)


Should we force the called to state how large is the destination (in bytes)?

@KrzysztofCwalina From my perspective, as the end user having elementsCount is more usefull than sizeInBytes. Mostly because you need the sizeof !!T to calculate the sizeInBytes. It's part of IL but not available in C#. I have added the SizeOf<T>() to JitHelpers so CLR can take care of this part instead of leaving it for the end user.

What is your opinion on that?

To me it's about ensuring that the caller does not cause buffer overruns because they misjudged the size of the buffer needed to fit the elements. It's much less likely that they don't know how many bytes they have in the destination.

@jkotas, any opinions on this?

I think it is confusing either way. It may be better to not have this unsafe API at all - the fewer APIs that take unmanaged pointers the better, and instead make people to use the other overload that takes Span - it more clearly expresses the intent:

TryCopyTo(Span<int>(pBuffer, nElements)).

adamsitnik · 2016-08-31T22:02:53Z

@jkotas @KrzysztofCwalina overlapping support added

adamsitnik · 2016-09-02T10:26:39Z

@jkotas @KrzysztofCwalina I have inlined the code contracts as Jan suggested, also added info about exceptions to summaries.

Unanswered questions:

Should I rename CreateArray to ToArray?
Should I remove unsafe bool TryCopyTo(void* destination, int elementsCount) or change elementsCount to sizeInBytes?
Should I add implicit cast operator of Array to Span?

Also new thing: @KrzysztofCwalina can I add Cast<T, TU> method implementation to Span as it is in corefxlab? With CLR's helpers I could constrain T & TU to be blittable or value types without references (runtime chcecks).

adamsitnik · 2016-09-12T09:06:28Z

@KrzysztofCwalina @jkotas could you answer the questions and/or accept the PR?

davidfowl · 2016-09-12T09:10:42Z

I'm going to answer those questions even though it probably doesn't matter:

Should I rename CreateArray to ToArray?

Yes. Matches with the rest of the CLR nomenclature. ToArray() allocates which is the reason it's called CreateArray() (to make sure you know what you're doing).

Should I remove unsafe bool TryCopyTo(void* destination, int elementsCount) or change elementsCount to sizeInBytes?

Remove it. Span all the things!

Should I add implicit cast operator of Span to Array?

Yes. It matches this dotnet/corefxlab@97afeb6.

adamsitnik · 2016-09-12T09:16:57Z

@davidfowl thanks!

omariom · 2016-09-12T10:20:23Z

Should I add implicit cast operator of Span to Array?

You mean array to span?

adamsitnik · 2016-09-12T11:37:59Z

You mean array to span?

@omariom yes, you are right. I will update the description

…nlySpan

adamsitnik · 2016-09-12T13:39:05Z

@jkotas @KrzysztofCwalina Done. Please let me know if it requires any further changes.

KrzysztofCwalina · 2016-09-12T15:50:09Z

looks good to me!

jkotas · 2016-09-12T16:22:03Z

Thank you!

Add TryCopyTo methods to Spans API #5857

c901b00

dnfclas added the cla-already-signed label Aug 8, 2016

omariom reviewed Aug 8, 2016
View reviewed changes

unit tests for copying slices + adopted to code review comments

abd7de8

jamesqo reviewed Aug 11, 2016
View reviewed changes

adamsitnik added 3 commits August 27, 2016 21:45

use methodTable->ContainsPointers() only as ArrayNative::ArrayCopyNoT…

faf8343

…ypeCheck does

bug fix: destination can be longer than source

3a6d4d2

added two extra test to verify what works

4336b05

adamsitnik added 3 commits August 29, 2016 17:41

Merge remote-tracking branch 'upstream/SpanOfT' into spanFastCopy

3e12afc

Conflicts: src/mscorlib/src/System/Runtime/CompilerServices/jithelpers.cs src/vm/jitinterface.cpp src/vm/mscorlib.h tests/src/CoreMangLib/system/span/BasicSpanTest.cs

generate Slice IL helpers in thread safe way

26dde01

could not store GetByRef's result in local variable and reuse it for …

ed54601

…whole loop, it did not work well with structs with pointers

jkotas reviewed Aug 29, 2016
View reviewed changes

adamsitnik added 2 commits August 30, 2016 15:19

Slice(int start) method implementation

42bf5e0

Merge remote-tracking branch 'upstream/SpanOfT' into spanFastCopy

c1e1fd1

Conflicts: src/mscorlib/src/System/Span.cs

jkotas reviewed Aug 30, 2016
View reviewed changes

KrzysztofCwalina reviewed Aug 30, 2016
View reviewed changes

copying spans: support for overlapping

49c52f4

inlined and commented code contracts

a7ef699

removing unsafe methods, added implicit cast op array to Span & ReadO…

928f035

…nlySpan

jkotas merged commit d83919c into dotnet:SpanOfT Sep 12, 2016

jkotas pushed a commit to jkotas/coreclr that referenced this pull request Oct 29, 2016

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

fa74e05

jkotas pushed a commit to jkotas/coreclr that referenced this pull request Oct 29, 2016

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

5d22977

jkotas pushed a commit to jkotas/coreclr that referenced this pull request Oct 29, 2016

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

c3a8f62

jkotas pushed a commit to jkotas/coreclr that referenced this pull request Oct 29, 2016

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

4b52905

jkotas pushed a commit to jkotas/coreclr that referenced this pull request Oct 31, 2016

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

3ba437e

Conversation

adamsitnik commented Aug 8, 2016

Uh oh!

omariom Aug 8, 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

Uh oh!

jkotas commented Aug 8, 2016

Uh oh!

jkotas commented Aug 8, 2016

Uh oh!

adamsitnik commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Aug 27, 2016

Uh oh!

omariom commented Aug 27, 2016

Uh oh!

adamsitnik commented Aug 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Aug 29, 2016

Uh oh!

adamsitnik commented Aug 29, 2016

Uh oh!

jkotas commented Aug 29, 2016

Uh oh!

jkotas commented Aug 29, 2016

Uh oh!

adamsitnik commented Aug 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Aug 29, 2016

Uh oh!

jkotas Aug 29, 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!

adamsitnik commented Aug 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Aug 31, 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!

jkotas commented Aug 30, 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!

jkotas Aug 31, 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!

adamsitnik commented Aug 31, 2016

Uh oh!

adamsitnik commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsitnik commented Sep 12, 2016

Uh oh!

davidfowl commented Sep 12, 2016

Uh oh!

adamsitnik commented Sep 12, 2016

Uh oh!

omariom Aug 8, 2016 •

edited

Loading

adamsitnik commented Aug 28, 2016 •

edited

Loading

adamsitnik commented Aug 29, 2016 •

edited

Loading

jkotas Aug 29, 2016 •

edited

Loading

jkotas Aug 31, 2016 •

edited

Loading

jkotas Aug 31, 2016 •

edited

Loading

adamsitnik commented Sep 2, 2016 •

edited

Loading