Extract portable/managed code from Array for sharing with Mono and CoreCLR by marek-safar · Pull Request #3201 · dotnet/corert

marek-safar · 2017-04-04T11:00:56Z

Includes few missing GetLowerBound calls from CoreCLR version

ghost · 2017-04-04T13:41:40Z

In keeping with our filenaming conventions (https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/shared/README.md), please rename...

Array.Portable.cs => Array.cs
Array.cs => Array.CoreRT.cs

Thanks!

ghost · 2017-04-04T13:45:54Z

Array.Initialize() is not functioning on CoreRt and is inherently unportable. It should stay in the non-portable partition.

MichalStrehovsky · 2017-04-04T13:45:20Z

src/System.Private.CoreLib/src/System/Array.Portable.cs

+            // Project N port note: On the desktop, this api is a nop unless the array element type is a value type with
+            // an explicit nullary constructor. Such a type cannot be expressed in C# so Project N does not support this.
+            // The ILC toolchain fails the build if it encounters such a type.
+            return;


This won't be sharable once CoreRT implements the right semantics.

Moved to CoreRT version

MichalStrehovsky · 2017-04-04T13:50:28Z

src/System.Private.CoreLib/src/System/Array.Portable.cs

+            int endIndex = startIndex + count;
+
+            // See comment above Array.GetComparerForReferenceTypesOnly for details
+            EqualityComparer<T> comparer = GetComparerForReferenceTypesOnly<T>();


Places that call GetComparerForReferenceTypesOnly rely on deep compiler magic that is not even portable between our two CoreRT compilers right now.

Moved to CoreRT version

There are still places in Array.cs left that call into this.

MichalStrehovsky · 2017-04-04T13:54:34Z

src/System.Private.CoreLib/src/System/Array.Portable.cs

+                    Array.CopyImplReferenceArrayToValueTypeArray(objItems, index, items, index, length, reliable: false);
+                }
+#else
+                Object[] objKeys = keys as Object[];


This is going to break us on the TFS side because Project N (the AOT compiler for Universal Windows Apps) doesn't define CORERT.

That something we should fix.

CORERT - defined in both TFS side and GitHub side

PROJECTN - defined in TFS only

For now, any ifdefs need to be #if CORERT || PROJECTN.

Or even better - move anything with ifdefs to the non-portable part.

Moved to CoreRT part

MichalStrehovsky · 2017-04-04T13:57:46Z

src/System.Private.CoreLib/src/System/Array.Portable.cs

+                {
+                    if (_index < 0) throw new InvalidOperationException(SR.InvalidOperation_EnumNotStarted);
+                    if (_index >= _endIndex) throw new InvalidOperationException(SR.InvalidOperation_EnumEnded);
+                    return _array.GetValueWithFlattenedIndex_NoErrorCheck(_index);


ArrayEnumerator differs from CoreCLR at this place (we avoid a bunch of checks this way), but it makes the enumerator rather non-portable (see my pending change to it in #3192).

This should not matter as GetValueWithFlattenedIndex_NoErrorCheck is in implementation specific part

Yeah, but as the name suggest, it doesn't do error checking, which means it doesn't check if the element type is an unmanaged pointer. If I get my #3192 signed off before you do your pull request, it won't be portable...

@MichalStrehovsky Is https://github.com/dotnet/corert/pull/3192/files#diff-011953c0f2212e0d4fbd0fd14df7467fR2884 CoreRT limitation ?

CoreRT limitation ?

You can't call get_Current on the enumerator of e.g. int*[]. I would think Mono blocks this too (CLR does). Pointer types cannot be boxed.

moved it to CoreRT part

ghost · 2017-04-04T14:06:16Z

You should probably compare each method with the CoreCLR version before putting in in the shared part. Even if the code compiles and runs, you may lose optimizations that CoreRT wasn't able to take advantage of (in particular, around the sorting and searching apis where CoreRT was hobbled by the inability to create generic instantiations on the fly with assurance of success.)

There's also the "throw" vs. ThrowHelper issue (another optimization thing on CoreCLR.)

marek-safar · 2017-04-04T14:42:56Z

@atsushikan files renamed

I'll leave it to CoreCLR folks to decide what to do with ThrowHelper optimization

jkotas · 2017-04-04T15:28:54Z

decide what to do with ThrowHelper optimization

We should deal with that separately from this PR - once we actually move it to the shared partition.

MichalStrehovsky · 2017-04-04T17:35:40Z

I can't comment on the diff because this is unchanged, but

private static bool StructOnlyEquals<T>(T left, T right)

Is the same kind of deep compiler magic as GetComparerForReferenceTypesOnly that should not be portable (neither any of the references to it).

It also seems like we don't need ComparerAsComparerT in the common code.

MichalStrehovsky · 2017-04-04T18:00:53Z

If you make your change a rename Array.cs -> Array.CoreRT.cs followed by starting a new Array.cs from scratch, it should merge clean with the pull request I just merged. The interesting parts (and majority of the code) is in Array.CoreRT.cs so that's where I would want to see uninterrupted history anyway.

marek-safar · 2017-04-05T06:01:56Z

@MichalStrehovsky Done. Although I could move the new common bits to shared and avoid the rename altogether

MichalStrehovsky · 2017-04-05T17:18:55Z

@marek-safar The private static bool StructOnlyEquals<T>(T left, T right) is still in Array.cs, but should be in Array.CoreRT.cs.

I'm not sure if it's worth having Empty<T>() in the common subset. I've been thinking about potentially optimizing this so that it doesn't force existence of a bunch of data structures per each "T", and can be inlined by RyuJIT. There's no interesting logic in that method.

I don't know what the policy is for moves to /shared/ (do we need to follow more cleanup rules?). I'm not opposed to it, but I've been detached from those efforts. @jkotas?

Other than that, looks good! Thanks!

jkotas · 2017-04-06T03:24:31Z

I don't know what the policy is for moves to shared

The best way to do it is to do similar refactoring in both CoreCLR and CoreRT, so that the file being moved is same or pretty close in both. Then move the file to shared in one repo, wait for the automatic PR to get generated in the other repo and amend it with a commit to delete the unshared copy of the file in other repo.

I do not think it makes sense to move Array.cs to shared as part of this PR.

…oreCLR Includes few missing GetLowerBound calls from CoreCLR version

marek-safar · 2017-04-06T08:36:17Z

@MichalStrehovsky moved both to CoreRT version

MichalStrehovsky · 2017-04-06T16:57:29Z

@dotnet-bot test OSX Release

dnfclas added the cla-already-signed label Apr 4, 2017

marek-safar force-pushed the array branch 2 times, most recently from 477dc78 to 1884826 Compare April 4, 2017 11:54

MichalStrehovsky reviewed Apr 4, 2017

View reviewed changes

marek-safar force-pushed the array branch from 1884826 to c9966ea Compare April 4, 2017 14:37

marek-safar force-pushed the array branch 2 times, most recently from 2c6262c to 90fe366 Compare April 4, 2017 17:54

marek-safar force-pushed the array branch 2 times, most recently from 99f5538 to efdb06e Compare April 4, 2017 18:36

Rename Array.cs to Array.CoreRT.cs

b351953

marek-safar force-pushed the array branch 2 times, most recently from 666ebb7 to 241f8bd Compare April 4, 2017 19:07

Extract portable/managed code to Array.cs for sharing with Mono and C…

560569e

…oreCLR Includes few missing GetLowerBound calls from CoreCLR version

marek-safar force-pushed the array branch from 241f8bd to 560569e Compare April 6, 2017 08:35

MichalStrehovsky merged commit 1fc9809 into dotnet:master Apr 6, 2017

Conversation

marek-safar commented Apr 4, 2017

Uh oh!

ghost commented Apr 4, 2017

Uh oh!

ghost commented Apr 4, 2017

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!

jkotas Apr 4, 2017 • 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!

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!

ghost commented Apr 4, 2017

Uh oh!

marek-safar commented Apr 4, 2017

Uh oh!

jkotas commented Apr 4, 2017

Uh oh!

MichalStrehovsky commented Apr 4, 2017

Uh oh!

MichalStrehovsky commented Apr 4, 2017

Uh oh!

marek-safar commented Apr 5, 2017

Uh oh!

MichalStrehovsky commented Apr 5, 2017

Uh oh!

jkotas commented Apr 6, 2017

Uh oh!

marek-safar commented Apr 6, 2017

Uh oh!

MichalStrehovsky commented Apr 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jkotas Apr 4, 2017 •

edited

Loading