Span factoring #26667

tarekgh · 2018-01-30T01:22:06Z

This change is moving the common span code to a shared files

Fixes #26611

tarekgh · 2018-01-30T01:22:52Z

CC @jkotas @ahsonkhan @GrabYourPitchforks

tarekgh · 2018-01-30T01:24:24Z

@safern FYI as I expect this will break coreclr when we open the mirroring PR there. please tag me on that PR when we have it.

safern · 2018-01-30T01:26:00Z

I will. Thanks for letting me know.

ahsonkhan · 2018-01-30T01:36:28Z

src/Common/src/CoreLib/System/ReadOnlySpan.Shared.cs

+                [MethodImpl(MethodImplOptions.AggressiveInlining)]
+                get
+                {
+                    // TODO https://github.com/dotnet/corefx/issues/24105:


This is done and hence the implementation can be cleaned up to be identical.

cool. I'll fix that

ahsonkhan · 2018-01-30T01:37:50Z

src/Common/src/CoreLib/System/ReadOnlySpan.cs

-        /// </summary>
-        public int Length
-        {
-            [NonVersionable]


Why did we have some properties marked as NonVersionable before?

cc @jkotas

Do we need using System.Runtime.Versioning; in this file anymore?

This is important for R2R code quality (the attribute allows cross-module inlining for these properties). We should keep the attributes for the Fast version.

jkotas · 2018-01-30T01:36:49Z

src/System.Memory/src/System.Memory.csproj

@@ -112,6 +112,8 @@
    <Compile Include="$(CommonPath)\CoreLib\System\Runtime\InteropServices\MemoryMarshal.cs" />
    <Compile Include="System\ReadOnlySpan.cs" />
    <Compile Include="System\Span.cs" />
+    <Compile Include="$(CommonPath)\CoreLib\System\Span.Shared.cs" />


The shared implementation should have no suffix. The fast-span specific implementation should have .Fast.cs suffix. It is the convention that we have been using for this so far.

ok, will rename the files then.

{Readonly}Span.cs -> {ReadOnly}Span.Fast.cs
{Readonly}Span.Shared.cs -> {Readonly}Span.cs

tarekgh · 2018-01-30T02:02:57Z

@jkotas renaming the files as desired confused git. but in general, I didn't change anything in the span code and I should be addressed all comments.

other idea would be do the renaming in 2 steps (I mean in 2 PR's)

jkotas · 2018-01-30T02:10:24Z

I still see .Shared.cs files in the PR.

jkotas · 2018-01-30T02:11:48Z

src/Common/src/CoreLib/System/ReadOnlySpan.cs

        /// <summary>
        /// The number of items in the read-only span.
        /// </summary>
        public int Length
        {
+#if !FEATURE_PORTABLE_SPAN


3 ifdefs to shared 150 files. Is it worth it?

Any future shared code will go to this shared files and hopefully, down the road, we may be able to get rid of the #if's.

Down the road, we will likely stop building the slow Span. System.Memory is partial OOB. Once it ships, the surface of the portable version will be locked down. We won't be able to add to it.

I am fine if you think it is not worth it to split the files and I can close this PR. I don't have a strong feeling about it either. @ahsonkhan asked if we can refactor the common code to the shared place. so, I am fine either way :-)

I don't have a strong feeling about it either.

Same here.

ok, as I have the changes ready and passing the CI, I'll continue with this PR. let me know if you have any more comments before I merge it. thanks for your comments.

@ahsonkhan asked if we can refactor the common code to the shared place. so, I am fine either way :-)

I think the enumerator, at the very least, should be shared, since it is already contained enough that it can be extracted out easily.

Down the road, we will likely stop building the slow Span. System.Memory is partial OOB.

Given this, extracting the other common code is less important, but I think it makes maintainability a bit easier.

jkotas · 2018-01-30T02:13:19Z

src/System.Memory/src/System.Memory.csproj

@@ -112,6 +112,8 @@
    <Compile Include="$(CommonPath)\CoreLib\System\Runtime\InteropServices\MemoryMarshal.cs" />
    <Compile Include="System\ReadOnlySpan.cs" />


These should be named .Portable.cs to follow the naming convention.

GrabYourPitchforks · 2018-01-30T07:13:50Z

LGTM - I don't have any review feedback beyond what has already been provided.

tarekgh · 2018-01-30T20:32:54Z

test NETFX x86 Release Build

tarekgh · 2018-01-30T21:31:21Z

@jkotas are you ok to merge?

jkotas · 2018-01-30T22:23:19Z

I would wait for the backlog of the CoreCLR Span commits to clear through the mirror before merging this one. Otherwise, I am fine with this.

tarekgh · 2018-01-31T23:59:58Z

test NETFX x86 Release Build

tarekgh · 2018-02-01T00:43:12Z

the failed test in the CI is tracked by the issue https://github.com/dotnet/corefx/issues/26725

tarekgh · 2018-02-01T03:02:47Z

@jkotas is it ok to merge now? or there are more changes coming from coreclr? I already merged with the latest we got from coreclr?

jkotas

Yes, I think it is fine.

jkotas · 2018-02-01T05:09:24Z

The CI results have quite a bit of red though.

jkotas · 2018-02-01T05:09:32Z

@dotnet-bot test this please

jkotas · 2018-02-01T08:06:36Z

Actually, I was wrong - there were still pending Span changes waiting to be mirrored. I have just merge them and there is conflict again. I am sorry about the conflicts...

tarekgh · 2018-02-01T16:33:20Z

no worries. I'll try to merge one more time. Thanks @jkotas

…ctoring

jkotas · 2018-02-01T17:12:47Z

@Anipik @safern There several PRs in CoreCLR (one of them is in Span) that the mirror is not picking up. Could you please take a look?

safern · 2018-02-01T18:31:34Z

@Anipik @safern There several PRs in CoreCLR (one of them is in Span) that the mirror is not picking up. Could you please take a look?

The machine seems to be doing Windows Updates. So the mirrors are down. Once it is done updating we will restart them.

tarekgh · 2018-02-01T19:12:29Z

@jkotas can we merge it now?

jkotas · 2018-02-01T19:52:27Z

The mirror was stuck. There are still pending Span changes in CoreCLR that this will conflict with.

If you merge now, you will get conflicts in the mirror job once the mirror starts running again...

tarekgh · 2018-02-01T20:06:54Z

I missed the mirroring part. I'll keep waiting to clear the mirroring and then remerge.

safern · 2018-02-01T20:51:19Z

I missed the mirroring part. I'll keep waiting to clear the mirroring and then remerge.

The machine is not responding and I can't RDP to it. I'm working with the service engineers to get it fixed. I'll keep you guys posted.

jkotas · 2018-02-02T03:03:49Z

@tarekgh This should be ready for re-merge now.

tarekgh · 2018-02-02T17:19:30Z

thanks @jkotas. I have done the merge.

tarekgh · 2018-02-02T18:22:28Z

The failure in NETFX x86 Release Build is the known issue #26770

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Commit migrated from dotnet/corefx@a68803c

ahsonkhan reviewed Jan 30, 2018

View reviewed changes

jkotas suggested changes Jan 30, 2018

View reviewed changes

jkotas reviewed Jan 30, 2018

View reviewed changes

Span factoring

583f824

tarekgh force-pushed the SpacFactoring branch from 31fbfff to 583f824 Compare January 31, 2018 23:21

jkotas mentioned this pull request Feb 1, 2018

Change the Span ToString semantics to return the contents for T=char dotnet/coreclr#16143

Merged

jkotas approved these changes Feb 1, 2018

View reviewed changes

tarekgh added 2 commits February 1, 2018 08:49

resolve merge conflicts

f30c4d7

Merge branch 'master' of https://github.com/dotnet/corefx into SpacFa…

26f5661

…ctoring

another merge conflict resolving

50fa3ea

jkotas merged commit a68803c into dotnet:master Feb 2, 2018

dotnet-bot pushed a commit to dotnet/coreclr that referenced this pull request Feb 2, 2018

Span factoring (dotnet/corefx#26667)

6dca6be

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

tarekgh deleted the SpacFactoring branch February 2, 2018 22:45

tarekgh added a commit to dotnet/coreclr that referenced this pull request Feb 3, 2018

Span factoring (dotnet/corefx#26667)

639ef5d

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Feb 3, 2018

Span factoring (dotnet/corefx#26667)

30548ea

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

tarekgh added a commit to dotnet/corert that referenced this pull request Feb 3, 2018

Span factoring (dotnet/corefx#26667)

04402dd

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

karelz added this to the 2.1.0 milestone Feb 4, 2018

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Span factoring (dotnet/corefx#26667)

93922f3

Commit migrated from dotnet/corefx@a68803c

		@@ -112,6 +112,8 @@
		<Compile Include="$(CommonPath)\CoreLib\System\Runtime\InteropServices\MemoryMarshal.cs" />
		<Compile Include="System\ReadOnlySpan.cs" />

Span factoring #26667

Span factoring #26667

Conversation

tarekgh commented Jan 30, 2018 • edited Loading

tarekgh commented Jan 30, 2018

tarekgh commented Jan 30, 2018

safern commented Jan 30, 2018

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

tarekgh commented Jan 30, 2018

jkotas commented Jan 30, 2018

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

GrabYourPitchforks commented Jan 30, 2018

tarekgh commented Jan 30, 2018

tarekgh commented Jan 30, 2018

jkotas commented Jan 30, 2018 • edited Loading

tarekgh commented Jan 31, 2018

tarekgh commented Feb 1, 2018

tarekgh commented Feb 1, 2018

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Feb 1, 2018

jkotas commented Feb 1, 2018

jkotas commented Feb 1, 2018

tarekgh commented Feb 1, 2018

jkotas commented Feb 1, 2018

safern commented Feb 1, 2018

tarekgh commented Feb 1, 2018

jkotas commented Feb 1, 2018

tarekgh commented Feb 1, 2018

safern commented Feb 1, 2018

jkotas commented Feb 2, 2018

tarekgh commented Feb 2, 2018

tarekgh commented Feb 2, 2018

tarekgh commented Jan 30, 2018 •

edited

Loading

jkotas commented Jan 30, 2018 •

edited

Loading