Consolidate System.Memory code to shared folder by tarekgh · Pull Request #26393 · dotnet/corefx

tarekgh · 2018-01-17T19:50:09Z

This change is removing the duplicate codes from System.Memory and keep only one copy under the shared folder to be easier to edit such code in one place and get reflected on the other repos.

tarekgh · 2018-01-17T19:52:17Z

@ahsonkhan @jkotas could you please have a look?

CC @joshfree @KrzysztofCwalina @GrabYourPitchforks @atsushikan

tarekgh · 2018-01-17T19:53:22Z

@safern this change will break coreclr when get mirrored. so, please ping me when the mirroring PR get opened so I can fix the breaks before we merge it there. I expect this break on corert too.

safern · 2018-01-17T19:54:53Z

Ok. I will keep an eye.

jkotas · 2018-01-17T20:00:36Z

                }
                else if (typeof(T) == typeof(char) && _object is string s)
                {
+#if CORECLR || CORERT


We should have a different ifdef for this so that this is just one condition.

It can be #if netstandard or #if FEATURE_PORTABLE_SPAN. We have prior art for both these.

jkotas · 2018-01-17T20:04:23Z

                ThrowHelper.ThrowArgumentNullException(ExceptionArgument.ownedMemory);
            if (index < 0 || length < 0)
-                ThrowHelper.ThrowArgumentOutOfRangeException();
+                ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);


This does not have argument called start.

jkotas · 2018-01-17T20:05:30Z

                ThrowHelper.ThrowArgumentNullException(ExceptionArgument.array);
            if (default(T) == null && array.GetType() != typeof(T[]))
-                ThrowHelper.ThrowArrayTypeMismatchException();
+                ThrowHelper.ThrowArrayTypeMismatchException_ArrayTypeMustBeExactMatch(typeof(T));


Passing in typeof(T) has non-trivial overhead here. I do not think it is worth doing for this rare exception.

ahsonkhan · 2018-01-17T20:09:30Z

 namespace System
 {
+    /// <summary>
+    /// Memory represents a contiguous region of arbitrary memory similar to Span.


nit: Use decorator tag cref when referring to Span, similar to the comment in ReadOnlyMemory.cs

/// <summary> /// Represents a contiguous region of memory, similar to <see cref="Span{T}"/>. /// Unlike <see cref="Span{T}"/>, it is not a byref-like type. /// </summary>

ahsonkhan · 2018-01-17T20:10:47Z


+
+#if CORECLR || CORERT
        public static ref T GetReference<T>(Span<T> span) => ref span._pointer.Value;


Add xml comments here as well.

ahsonkhan · 2018-01-17T20:12:55Z

Would this using directive work for portable span or does it need to be within ifdef?
https://github.com/dotnet/corefx/pull/26393/files#diff-85c2810af721b319d724938d5506641dR11

 using Internal.Runtime.CompilerServices;

jkotas · 2018-01-17T20:16:28Z

using Internal.Runtime.CompilerServices; needs to be ifdefed.

ahsonkhan · 2018-01-17T20:16:45Z

    <Compile Include="..\Common\src\System\MutableDecimal.cs" />
  </ItemGroup>
  <ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true'">
+    <Compile Include="$(CommonPath)\CoreLib\System\System\Memory.cs" />


extra \System within the paths

here and elsewhere

should be:
"$(CommonPath)\CoreLib\System\Memory.cs"

ahsonkhan

otherwise, lgtm

tarekgh · 2018-01-17T21:12:26Z

Thanks @ahsonkhan for your review.

@jkotas I have addressed all your comments. please let me know if you have more comments. Thanks.

jkotas · 2018-01-17T21:22:15Z

-            if (index < 0 || length < 0)
-                ThrowHelper.ThrowArgumentOutOfRangeException();
+            if (index < 0)
+                ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);


This internal constructor and the callers are validating the arguments already. Do we need to validate the arguments here again?

These constructors are hot paths, with AggresiveInline. Any unnecessary cruft here impacts microbencmarks; and has multiplicative effect on code size.

Context for the checks being there, from @stephentoub:
https://github.com/dotnet/coreclr/pull/13583/files/f538e879ed80d82c155270796d2ab20bdd63dbe5#r135248195

I am seeing there is no strong opinion to keep the checks even in the comment that @ahsonkhan pointed at. do you agree with removing the checks (for index and length)?

It is odd to have a partial argument validation. I think we should do no validation here, and let the callers take care of it as necessary.

ahsonkhan · 2018-01-17T21:23:08Z

    <DefineConstants Condition="'$(IsPartialFacadeAssembly)' != 'true'">$(DefineConstants);netstandard</DefineConstants>
    <DefineConstants Condition="'$(TargetGroup)'=='netcoreapp'">$(DefineConstants);netcoreapp</DefineConstants>
    <DefineConstants Condition="'$(TargetGroup)'=='netstandard1.1'">$(DefineConstants);netstandard11</DefineConstants>
+    <DefineConstants>$(DefineConstants);FEATURE_PORTABLE_SPAN</DefineConstants>


I think there needs to be a condition for this constant (either IsPartialFacadeAssembly != true, or use the existing netstandard).

I thought about that but found we include only the needed files when building the portable library. I can add the condition but I am not seeing this is necessary. I'll add the condition anyway.

jkotas · 2018-01-17T21:23:36Z

            if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
            {
-                ThrowHelper.ThrowArgumentOutOfRangeException();
+                ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);


The argument name is not accurate - it can be length that is wrong. I think we should not bother with supplying the argument name here.

this is the code I picked from the System.Memory in corefx but I agree with you. I'll fix that.

tarekgh · 2018-01-17T22:33:49Z

The failures are known tracked issue https://github.com/dotnet/corefx/issues/26382

ViktorHofer · 2018-01-17T22:40:54Z

    <Compile Include="..\Common\src\System\MutableDecimal.cs" />
  </ItemGroup>
  <ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true'">
+    <Compile Include="$(CommonPath)\CoreLib\System\Memory.cs" />


should we introduce a variable for the shared folder? e.g. $(SharedPath) or $(SharedCommonPath)

I am not sure if it is worth it.

Replacing all existing paths starting with $(CommonPath)\CoreLib works if we ever change the place of the mirror. So I'm fine with hardcoding it.

All these path variables just makes it harder for people to find the files. It's not like msbuild supports "hitting F12 to go to the definition of path variable."

Not a good tradeoff for the remote chance that the mirror location will change, especially since search-replace will work just fine in that case.

I agree. It's good that we talked about it.

tarekgh · 2018-01-17T23:11:58Z

@jkotas any more comments?

jkotas · 2018-01-17T23:20:34Z

-                ThrowHelper.ThrowArgumentNullException(ExceptionArgument.ownedMemory);
-            if (index < 0 || length < 0)
-                ThrowHelper.ThrowArgumentOutOfRangeException();



Nit: Extra line

It may be useful to add // No validation performed; caller must provide any necessary validation. comment here - similar to what it is in the other ctor.

jkotas

LGTM modulo nit

tarekgh · 2018-01-17T23:47:43Z

#25188

* Consolidate System.Memory code to shared folder This change is removing the duplicate codes from System.Memory and keep only one copy under the shared folder to be easier to edit such code in one place and get reflected on the other repos. * Address the review feedback * Addressing more feedback * More cleanup * remove empty line and added a comment Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

* Consolidate System.Memory code to shared folder (dotnet/corefx#26393) * Consolidate System.Memory code to shared folder This change is removing the duplicate codes from System.Memory and keep only one copy under the shared folder to be easier to edit such code in one place and get reflected on the other repos. * Address the review feedback * Addressing more feedback * More cleanup * remove empty line and added a comment Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Add missing throw helper methods used in the code we got from corefx * Update the exception helper * fix the break Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Consolidate System.Memory code to shared folder (dotnet/corefx#26393) * Consolidate System.Memory code to shared folder This change is removing the duplicate codes from System.Memory and keep only one copy under the shared folder to be easier to edit such code in one place and get reflected on the other repos. * Address the review feedback * Addressing more feedback * More cleanup * remove empty line and added a comment Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Add missing throw helper methods used in the code we got from corefx * Update the exception helper * fix the break

* Consolidate System.Memory code to shared folder (dotnet/corefx#26393) * Consolidate System.Memory code to shared folder This change is removing the duplicate codes from System.Memory and keep only one copy under the shared folder to be easier to edit such code in one place and get reflected on the other repos. * Address the review feedback * Addressing more feedback * More cleanup * remove empty line and added a comment Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Add missing throw helper methods used in the code we got from corefx * Update the exception helper * fix the break Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Consolidate System.Memory code to shared folder This change is removing the duplicate codes from System.Memory and keep only one copy under the shared folder to be easier to edit such code in one place and get reflected on the other repos. * Address the review feedback * Addressing more feedback * More cleanup * remove empty line and added a comment Commit migrated from dotnet/corefx@9340e12

Consolidate System.Memory code to shared folder

66f4167

This change is removing the duplicate codes from System.Memory and keep only one copy under the shared folder to be easier to edit such code in one place and get reflected on the other repos.

jkotas reviewed Jan 17, 2018

View reviewed changes

ahsonkhan reviewed Jan 17, 2018

View reviewed changes

ahsonkhan approved these changes Jan 17, 2018

View reviewed changes

Address the review feedback

84cd6af

jkotas reviewed Jan 17, 2018

View reviewed changes

ahsonkhan reviewed Jan 17, 2018

View reviewed changes

jkotas reviewed Jan 17, 2018

View reviewed changes

tarekgh added 2 commits January 17, 2018 13:47

Addressing more feedback

25e6b90

More cleanup

c70bf98

ViktorHofer reviewed Jan 17, 2018

View reviewed changes

jkotas reviewed Jan 17, 2018

View reviewed changes

jkotas approved these changes Jan 17, 2018

View reviewed changes

remove empty line and added a comment

eb61a7a

tarekgh merged commit 9340e12 into dotnet:master Jan 18, 2018

tarekgh deleted the ConsolidateSystemMemoryCodeToSharedFolder branch January 18, 2018 00:40

karelz added this to the 2.1.0 milestone Jan 20, 2018



		#if CORECLR \|\| CORERT
		public static ref T GetReference<T>(Span<T> span) => ref span._pointer.Value;

Uh oh!

Conversation

tarekgh commented Jan 17, 2018

Uh oh!

tarekgh commented Jan 17, 2018

Uh oh!

tarekgh commented Jan 17, 2018

Uh oh!

safern commented Jan 17, 2018

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!

ahsonkhan commented Jan 17, 2018

Uh oh!

jkotas commented Jan 17, 2018

Uh oh!

ahsonkhan Jan 17, 2018 • 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!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jan 17, 2018 • 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!

tarekgh commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tarekgh commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Jan 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ahsonkhan Jan 17, 2018 •

edited

Loading

ahsonkhan Jan 17, 2018 •

edited

Loading

tarekgh commented Jan 17, 2018 •

edited

Loading